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

SafeSocketHandle: avoid potential blocking of finalizer thread #41508

Merged
merged 10 commits into from
Sep 1, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Aug 28, 2020

When the Socket is Disposed, it attempts to make all on-going operations
abort. It does this in a loop, and decides there are no on-going operations
when the reference count becomes zero and the handle gets released.

SafePipeHandle holds a reference to SafeSocketHandle. This prevents the
reference count to drop to zero, and causes the dispose to loop infinitly.

When the Socket is disposed from the finalizer thread, it is no longer used
for operations and we can skip the loop. This avoids blocking the finalizer
thread when the reference count can't drop to zero.

Fixes #40301

@antonfirsov @geoffkizer ptal

cc @jaredpar

When the Socket is Disposed, it attempts to make all on-going operations
abort. It does this in a loop, and decides there are no on-going operations
when the reference count becomes zero and the handle gets released.

SafePipeHandle holds a reference to SafeSocketHandle. This prevents the
reference count to drop to zero, and causes the dispose to loop infinitly.

When the Socket is disposed from the finalizer thread, it is no longer used
for operations and we can skip the loop. This avoids blocking the finalizer
thread when the reference count can't drop to zero.
@ghost
Copy link

ghost commented Aug 28, 2020

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

@antonfirsov
Copy link
Member

I know it's a bit off-topic, but if we are already here, would make sense to remove comments about inner/outer handles in SafeSocketHandle.cs:

// It uses an inner and outer SafeHandle to do so. The inner
// SafeHandle holds the actual socket, but only ever has one // SafeHandle holds the actual socket, but only ever has one
// reference to it. The outer SafeHandle guards the inner // reference to it. The outer SafeHandle guards the inner
[.....]

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just nits, should be good once a test is added.

@tmds
Copy link
Member Author

tmds commented Aug 28, 2020

Anton, thanks for reviewing! I've addressed your feedback. Socket tests pass on my machine.

@antonfirsov
Copy link
Member

antonfirsov commented Aug 28, 2020

The OSX test failure is unrelated and tracked in #41511, the mono one seems to be a hang in System.Linq.Expressions.Tests.ArrayLengthTests.CheckByteArrayLengthTest:
https://helix.dot.net/api/2019-06-17/jobs/bac94c2c-2c76-4f75-9ee3-cfc1641cb18e/workitems/System.Linq.Expressions.Tests/console

Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@geoffkizer if you are also fine with the changes and all tests do pass, maybe we can even do the packport today?

@antonfirsov

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@antonfirsov
Copy link
Member

Inner loop test failure is #41078

@antonfirsov antonfirsov requested review from antonfirsov and geoffkizer and removed request for antonfirsov August 31, 2020 09:04
Copy link
Member

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test failure seems related:

Unhandled Exception:
System.NullReferenceException: Object reference not set to an instance of an object
   at System.Net.Sockets.Socket.Dispose(Boolean disposing) in /_/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 4262
   at System.Net.Sockets.Socket.Finalize() in /_/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 4367
[ERROR] FATAL UNHANDLED EXCEPTION: System.NullReferenceException: Object reference not set to an instance of an object
   at System.Net.Sockets.Socket.Dispose(Boolean disposing) in /_/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 4262
   at System.Net.Sockets.Socket.Finalize() in /_/src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs:line 4367

@antonfirsov

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@antonfirsov
Copy link
Member

OuterLoop test failures seem to be unrelated: #40798 #41530 #41531 #41606 #41607

@geoffkizer
Copy link
Contributor

We do have a test somewhere that ensures that the socket handle is closed when the Socket gets finalized, right?

This was the original problem that led us to adding logic to the Socket finalizer. We need to make sure we haven't broken that again.

@wfurt
Copy link
Member

wfurt commented Aug 31, 2020

NonDisposedSocket_SafeHandlesCollected ?

@antonfirsov antonfirsov merged commit 2ba6515 into dotnet:master Sep 1, 2020
@antonfirsov
Copy link
Member

/backport to release/5.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2020

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/234344348

@antonfirsov
Copy link
Member

/backport to release/5.0-rc2

@github-actions
Copy link
Contributor

github-actions bot commented Sep 2, 2020

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/236412175

antonfirsov pushed a commit that referenced this pull request Sep 8, 2020
…izer thread (#41747)

Backport of #41508 to release/5.0-rc2

* SafeSocketHandle: avoid potential blocking of finalizer thread

When the Socket is Disposed, it attempts to make all on-going operations
abort. It does this in a loop, and decides there are no on-going operations
when the reference count becomes zero and the handle gets released.

SafePipeHandle holds a reference to SafeSocketHandle. This prevents the
reference count to drop to zero, and causes the dispose to loop infinitly.

When the Socket is disposed from the finalizer thread, it is no longer used
for operations and we can skip the loop. This avoids blocking the finalizer
thread when the reference count can't drop to zero.

* When disposing from finalizer, fall back to ReleaseHandle

* Add test

* PR feedback

* Fix test

* PR feedback

* Refactor

* Refactor

* Log call to _handle.Dispose

* Handle null handle

Co-authored-by: Tom Deseyn <tom.deseyn@gmail.com>
@karelz karelz added this to the 6.0.0 milestone Sep 10, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SafeSocketHandle.CloseAsIs hanging in finalizer thread
6 participants