TLDR: Be very careful using captured closures and anonymous methods - they can leak memory when you don't expect. Also copy-and-paste code is bad.
So yesterday we had an issue where a long-running service within our platform suddenly started throwing StackOverflowExceptions with a new version. It was a blocker to the next release, so inevitably I got tasked with fixing the issue.
A quick profile of a locally running copy replicated the error (phew!) and pointed to one of our HTTP based service clients being at fault. This was confusing, as those components hadn't changed.
But what had changed was how those clients were being used - previously, they had accidentally been injected from the IoC container as singletons - now they were being injected as transients.
So why would the stack overflow?
It turns out that the service clients all had copy-and-paste code used to ensure that a custom JSON converter was added to the default JSONSerializerSettings used by the Newtonsoft JSON library, and that code was leaking memory and ultimately causing the StackOverflowException.
But how? Here's the old code:
Pretty straightforward - and a static method in a static class shouldn't leak?
WRONG!
On lines 8 and 9 we're capturing a closure (defaultSettings) and then using that within an anonymous method that we're assigning back to JsonConvert.DefaultSettings.
Because of the captured closure, the compiler couldn't make the anonymous method truly static - so you get a new instance every time the helper method is called - and those instances are pinned (again because of the captured closure) so that they can't be garbage collected.
Since this helper was being called every time a service client was being instantiated, and the clients were no longer singletons, the profile was showing tens of thousands of instances in memory of Func<JsonSerializerSettings>. Not good.
The solution is to be rather more defensive in adding the converter. Here's the corrected version:
We create our own default JsonSerializerSettings instance and a genuinely static Func<JsonSerializerSettings> helper method on lines 7 & 8.
If JsonConvert.DefaultSettings is null, we assign that helper (within a double-checked lock for safety) on line 18.
Finally, we use whatever helper factory was assigned to get the default JsonSerializerSettings instance (line 23) and add our converter if needed - again within a double-checked lock (line 32).
Re-running the repro resulted in exactly 1 instance of Func<JsonSerializerSettings> being created - and our bug is fixed.
The moral of the story? Watch out for anonymous methods and captured closures - they can bite!
1 comment:
Owning a reference prevents the garbage collector from collecting the referenced object but doesn't prevent it from being relocated.
Pinning means fixing an object to that place in memory preventing the grabage collector from relocating it (and collecting it, obviously): https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/fixed-statement
Post a Comment