Fix Client memory leak caused by not detaching UnhandledException event handler #391
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.
The event handler registered in the client's constructor closes over the client, and is never detached. The AppDomain.CurrentDomain UnhandledException event invocation list lives for the length of the program and acts as a GC root for any created clients (due to the addition of this handler), this means clients will get leaked even if they are no longer reachable from elsewhere in the program.
In this PR I have just removed the registration of the handler, I think it's fairly unusual for a client library to need to register a handler to receive notification for all unhandled exceptions in the current app domain. But please let me know if you disagree.
(On a separate but related note, I have also noticed that the underlying socket in Connection is not deterministically cleaned up, for example if Client.Create throws, connection is not disposed of and we rely on the finalizer of Socket to clean up underlying resources (at the moment the finalizer never runs because of the reference chain Client -> Connection -> Socket and the event handler never being detached)