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

Throw when deserializing Exceptions by reference, and avoid reference tracking for all Exception-derived types (#8628) #8698

Merged

Conversation

david-obee
Copy link
Contributor

@david-obee david-obee commented Nov 1, 2023

This adds the two suggestions @ReubenBond makes in this issue.

Throw an exception when deserializing an Exception-derived type by reference

When we have an Exception-derived type that has been serialized as a reference, we should throw an exception when trying to deserialize it with the ExceptionCodec, rather than silently returning null in it's place.

This scenario could (prior to this PR) come about when an exception is serialized using the code-gen serializer - say you add [GenerateSerializer] for your custom exception type, which is valid if you have certain fields you need serialized -, but this is stored in a field of type Exception. In that case, the code-gen serializer would by default (absent the SuppressReferenceTrackingAttribute) serialize the exception as a reference if it appears more than once. However, on deserialization the ExceptionCodec will be used, and will refuse to resolve that reference. This is what made me initially raise #8628.

Should that happen, this change just makes it clear in that case why deserialization fails, allowing the user to design around it.

Worth mentioning that I did have a test for this, but the next fix makes it much more difficult to test because it makes it harder to get into this scenario in the first place. I removed the test, as this is simple and exceptional logic, but it depends on how strict you want to be I think. It would be possible, but a pain to test.

Avoid reference tracking for all Exception-derived types in the first place

We take the behaviour that the ExceptionCodec has, which is to not serialize as instances of references when there are multiple references to the same Exception-derived object (reference tracking), and extend this for all Exception types, including those that don't use the ExceptionCodec (as described above). This should make less likely for us to get into the scenario above in the first place (that is if the system that serialized the message uses this change, if not then the the above change could still be relevant).

The trickier part here was implementing this to also cover exceptions that are using a surrogate type. I didn't want to make the change for exceptions that have [GenerateSerializer], but not have the same support for exceptions that have a surrogate, because I feel like that discrepancy would be more confusing than just not implementing this at all. To account for that, I also had to modify the SurrogateCodec, as it's responsible for reference tracking for surrogates.

I'm not sure how I feel about this change, it feels like it's baking in some knowledge about how the ExceptionCodec behaves into the SurrogateCodec. If we do want to have this ExceptionCodec behaviour for exceptions that aren't serialized by the ExceptionCodec however, I think this is necessary. I can completely see the argument though that this change adds too much complexity than it's worth. If you think that, then I'm happy to remove this part and just keep the previous change of throwing above instead.

Microsoft Reviewers: Open in CodeFlow

@ReubenBond ReubenBond merged commit 8f73d43 into dotnet:main Nov 11, 2023
19 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Dec 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants