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 Socket finalizer and ensure SafeSocketHandle finalization works correctly #43383

Open
geoffkizer opened this issue Oct 14, 2020 · 2 comments
Labels
area-System.Net.Sockets enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@geoffkizer
Copy link
Contributor

The Socket class has a finalizer, but it shouldn't need to have one. It should be able to rely on the finalizer of SafeSocketHandle to handle finalization when necessary, now that the logic in SafeSocketHandle is reliable as of 5.0.

Having both of these objects be finalizable is problematic since we can't control the order in which their finalizers are called, and this may lead to unexpected behavior. It's also just unneeded overhead in the GC.

I tested removing the finalizer, and everything works fine on Windows. However, on Linux the SafeSocketHandle is not properly collected. I believe this is because there's a reference to the SafeSocketHandle held (indirectly) by the SocketAsyncEngine dictionary. We probably need to make this a weak reference, or otherwise change the code so that the reference is not held.

@antonfirsov @stephentoub @tmds @wfurt

@ghost
Copy link

ghost commented Oct 14, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@tmds
Copy link
Member

tmds commented Oct 29, 2020

It would be nice if we can get rid of the finalizer on Socket. It is not causing any issues afaik.

@karelz karelz added the enhancement Product code improvement that does NOT require public API changes/additions label May 13, 2021
@karelz karelz modified the milestones: 6.0.0, Future May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Net.Sockets enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

3 participants