Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove serialization support in DynamicProxy #512

Closed

Conversation

stakx
Copy link
Member

@stakx stakx commented Jun 12, 2020

I am making this a draft to prevent accidental merging (although it's mostly ready); after all, we haven't decided yet whether to keep or retire serialization support in DynamicProxy. Until that time, consider this an "exploration" to help us better understand serialization in DynamicProxy.

This PR is perhaps best reviewed commit by commit, for the following reason:

#501 (comment) gave a summary of the different usages of FEATURE_SERIALIZATION in DynamicProxy, and appears to have been largely correct. I used it as a guideline to "undo" serialization from DynamicProxy, in the following order:

  1. Make all exception types serializable across all targets. (Update: Moved into a separate PR Make all exception types serializable #515.)

  2. Change DynamicProxy so generated proxy and invocation types are no longer serializable. (Those two kinds of generated types cannot be easily done piecemeal because they share a common base class, BaseProxyGenerator. Incorrect: Invocation type generators ultimately subtype IGenerator<T>, not BaseProxyGenerator.)

  3. Finally, abandon LoadAssemblyIntoCache and get rid of the requirement that "supporting types" (hooks, interceptors, interceptor selectors, mixins) should be made serializable.

The net result is that DynamicProxy code no longer makes use of FEATURE_SERIALIZATION —the remaining usages are inside DictionaryAdapter and the logging facility.

Closes #367.

@stakx stakx marked this pull request as draft June 12, 2020 18:13
@stakx stakx force-pushed the retire-dynamicproxy-serialization branch 3 times, most recently from a55d4e1 to 479a937 Compare June 12, 2020 18:41
@jonorossi
Copy link
Member

I won't get time to review this PR today, quite a lot to look at.

Did you want to submit your first bullet point to clean up exceptions (and maybe one other commit to fix a ifdef) as a separate pull request so we can get those in.

@stakx
Copy link
Member Author

stakx commented Jun 15, 2020

I did as you suggested and moved some things into separate PRs. The one for the incorrect compilation symbol is already merged, it was trivial and you've got enough boring conditional symbol obliteration PRs to review as it is. 😆

@stakx stakx force-pushed the retire-dynamicproxy-serialization branch from 09c5f9b to a1dac2c Compare June 15, 2020 20:04
@stakx
Copy link
Member Author

stakx commented Jan 30, 2021

Let's close this... at least for the time being. If we really do want to remove serialization support, we can always reactivate this PR.

@stakx stakx closed this Jan 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Proxying certain [Serializable] classes produces proxy types that fail PEVerify test
2 participants