Improve serialization perf and fix memory leak in SentryEvent #1693
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I ran into an issue wherein the
SentryExceptions
andSentryThreads
properties onSentryEvent
can return openJsonElement
objects instead of their actual properties. If theJsonDocument
holding those elements is not disposed, we get a memory leak. But also, if the document is disposed, we can't deserialize the data without getting anObjectDisposedException
. The root cause turned out to be an un-materializedIEnumerable
on those properties coming fromSentryEvent.FromJson
.The tests were hiding this, because we were using our
Json.Parse(string)
helper function, which was using.Clone()
, so the elements wouldn't have been disposed with the document. Since cloning is semi-costly, I revised the helper methods to avoid it, by passing in a factory method delegate instead and using it to return a concrete object instead of aJsonElement
.Only one test failed with the change to the JSON helpers, which was for the issue had already found on
SentryEvent
. Adding.ToList()
in the correct place made the test pass again.So - part fix, part perf improvement.