Socket: cancel on-going operations when Sockets with non-owning handle gets disposed. #72000

tmds · 2022-07-12T08:45:47Z

Fixes #70311.

@halter73 @wfurt @antonfirsov @stephentoub @dotnet/ncl ptal.

…e gets disposed.

ghost · 2022-07-12T08:46:00Z

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

Issue Details

Fixes #70311.

@halter73 @wfurt @antonfirsov @stephentoub @dotnet/ncl ptal.

Author:	tmds
Assignees:	-
Labels:	`area-System.Net.Sockets`
Milestone:	-

tmds · 2022-07-13T09:53:32Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs

+                // If we don't own the handle, cancel them explicitly.
+                if (!OwnsHandle)
+                {
+                    Interop.Kernel32.CancelIoEx(handle, null);


We're cancelling all operations that are performed against the handle here.
If there is another SafeSocketHandle for the same raw handle in this process, it may get its operations aborted by this.

For Unix we track the on-going operations performed against the SafeHandle in SocketAsyncContext so we're only canceling the operations that were performed against that handle.
On Unix we actually don't support different SafeSocketHandles to perform async operations on the same raw handle. SocketAsyncEngine holds a map from the raw handle to one SocketAsyncContext.

Can we imagine a sane user scenario where this could lead to an issue?

antonfirsov

Mostly questions and minor remarks.

antonfirsov · 2022-07-18T19:24:56Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs

-            : base(ownsHandle)
+            : base(ownsHandle: true) // To support canceling on-going operations we need to detect
+                                     // there are no more on-going operations.
+                                     // For that we the base-SafeHandle needs to be owning even


Suggested change

// For that we the base-SafeHandle needs to be owning even

// For that the base-SafeHandle needs to be owning even

antonfirsov · 2022-07-18T19:30:12Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Windows.cs

+                // If we don't own the handle, cancel them explicitly.
+                if (!OwnsHandle)
+                {
+                    Interop.Kernel32.CancelIoEx(handle, null);


Can we imagine a sane user scenario where this could lead to an issue?

antonfirsov · 2022-07-18T19:40:25Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.Unix.cs

+            // Don't disconnect sockets we don't own.
+            if (!OwnsHandle)


So if we don't own the handle we won't attempt to abort ongoing sync operations anymore on Socket / SafeSocketHandle close? I assume this is not an issue, because in the owning case we are doing it only to prevent an indefinitely blocking close() syscall (or a handle leak if we don't close)? Am I following this correctly?

We're not doing it because the function has side-effects which we don't want on a non-owning socket.

antonfirsov · 2022-07-18T19:45:20Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs

+                SocketError errorCode = OwnsHandle ? DoCloseHandle(abortive) : SocketError.Success;
                return ret = errorCode == SocketError.Success;


Nit: It's a bit weird to create a fake error code here. Maybe we can rewrite this part in the following form instead:

Suggested change

SocketError errorCode = OwnsHandle ? DoCloseHandle(abortive) : SocketError.Success;

return ret = errorCode == SocketError.Success;

return !OwnsHandle || DoCloseHandle(abortive) == SocketError.Success;

antonfirsov · 2022-07-18T19:55:55Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

@@ -3147,6 +3146,11 @@ protected virtual void Dispose(bool disposing)
                    if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.Dispose()");
                    handle.Dispose();
                }
+                else if (!handle.OwnsHandle)
+                {
+                    if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.CloseAsIs()");


Nit:
It might be beneficial to distinguish the different cases of Calling _handle.CloseAsIs() lines better, at least by logging if it's an abortive close.

antonfirsov · 2022-07-18T20:07:30Z

src/libraries/System.Net.Sockets/tests/FunctionalTests/Connect.cs

@@ -139,6 +149,8 @@ public async Task ConnectGetsCanceledByDispose(string addressString, bool useDns
            await RetryHelper.ExecuteAsync(async () =>
            {
                var client = new Socket(address.AddressFamily, SocketType.Stream, ProtocolType.Tcp);
+                using SafeSocketHandle? owner = ReplaceWithNonOwning(ref client, owning);


We do not keep the reference of the original Socket instance after this line. Couldn't this lead to an unexpected eager finalization of that instance while client and owner are still alive?

Otherwise it will be just an ignored failing close() syscall in the finalizer, right?

If we're replacing client (when owning is true) the original Socket gets:

socket.SafeHandle.SetHandleAsInvalid();

so it will no longer close the platform socket.

antonfirsov · 2022-07-18T20:10:46Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/SafeSocketHandle.cs

@@ -42,18 +42,19 @@ public sealed partial class SafeSocketHandle : SafeHandleMinusOneIsInvalid
        /// <param name="preexistingHandle">Handle to wrap</param>
        /// <param name="ownsHandle">Whether to control the handle lifetime</param>
        public SafeSocketHandle(IntPtr preexistingHandle, bool ownsHandle)
-            : base(ownsHandle)
+            : base(ownsHandle: true) // To support canceling on-going operations we need to detect


I think it would be wise to add tests verifying that a non-owned handle will not be closed by Dispose and finalization. (Couldn't find existing ones, maybe just missed them?)

That makes sense. I'll add those.

wfurt · 2022-07-25T21:05:45Z

src/libraries/System.Net.Sockets/src/System/Net/Sockets/Socket.cs

@@ -3147,6 +3146,11 @@ protected virtual void Dispose(bool disposing)
                    if (NetEventSource.Log.IsEnabled()) NetEventSource.Info(this, "Calling _handle.Dispose()");
                    handle.Dispose();
                }
+                else if (!handle.OwnsHandle)


nit: I personally don't like the negated booleans. I would use if (handle.OwnsHandle)) for the original block and add CloseAsIs() as new else.

wfurt

LGTM.
I was thinking if there is good way to verify that we leave socket AS_IS In spirit of #70311.
But I could not come up with only good example. Perhaps duplicate the handle and verify that the copy is still useable...?

This reverts commit 944910f.

antonfirsov · 2022-08-03T22:53:58Z

@wfurt can you please take a look on my latest changes?

Addressed some nits: e529b9b
Added test Ctor_Dispose_HandleClosedIfOwnsHandle: eef6bd2

If there are no objections to those changes, I will merge.

wfurt · 2022-08-04T16:39:41Z

test failures unrelated.

tmds · 2022-08-05T05:15:08Z

@antonfirsov, thanks for finishing this up!

Socket: cancel on-going operations when Sockets with non-owning handl…

c78092f

…e gets disposed.

dotnet-issue-labeler bot added the area-System.Net.Sockets label Jul 12, 2022

ghost added the community-contribution Indicates that the PR has been added by a community member label Jul 12, 2022

tmds mentioned this pull request Jul 12, 2022

Potential breaking change in socket close behavior in net6 over net5 on Linux #70311

Closed

tmds added 3 commits July 12, 2022 13:48

Try fix Windows test failure.

375728a

Fix Windows compilation failure.

a303e51

Only CancelIoEx when there is a ThreadPoolBoundHandle.

d19db4f

tmds commented Jul 13, 2022

View reviewed changes

karelz requested a review from antonfirsov July 18, 2022 07:59

antonfirsov reviewed Jul 18, 2022

View reviewed changes

wfurt reviewed Jul 25, 2022

View reviewed changes

wfurt approved these changes Jul 25, 2022

View reviewed changes

antonfirsov added 6 commits August 1, 2022 18:08

Merge branch 'main' into socket_non_owning_cancel_ongoing

6c6d48a

Ctor_Dispose_HandleClosedIfOwnsHandle

944910f

nits

e529b9b

Revert "Ctor_Dispose_HandleClosedIfOwnsHandle"

3ff0090

This reverts commit 944910f.

Ctor_Dispose_HandleClosedIfOwnsHandle (Windows-only)

eef6bd2

Merge branch 'main' into _socket_non_owning_cancel_ongoing

531707a

runfoapp bot mentioned this pull request Aug 4, 2022

System.Net.* cancellation tests fail in CI #72818

Closed

karelz assigned wfurt Aug 4, 2022

wfurt merged commit f6dec65 into dotnet:main Aug 4, 2022

karelz added this to the 7.0.0 milestone Aug 7, 2022

MihaZupan mentioned this pull request Aug 7, 2022

Sockets tests SendFileGetsCanceledByDispose and AcceptGetsCanceledByDispose - throw NotSupportedException - on AppleTV/iOS #73536

Open

ghost locked as resolved and limited conversation to collaborators Sep 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socket: cancel on-going operations when Sockets with non-owning handle gets disposed. #72000

Socket: cancel on-going operations when Sockets with non-owning handle gets disposed. #72000

tmds commented Jul 12, 2022

ghost commented Jul 12, 2022

tmds Jul 13, 2022

antonfirsov Jul 18, 2022

tmds Jul 28, 2022

antonfirsov left a comment

antonfirsov Jul 18, 2022

antonfirsov Jul 18, 2022

antonfirsov Jul 18, 2022 •

edited

Loading

tmds Jul 28, 2022

antonfirsov Jul 18, 2022

antonfirsov Jul 18, 2022

antonfirsov Jul 18, 2022

tmds Jul 28, 2022

antonfirsov Jul 18, 2022

tmds Jul 28, 2022

wfurt Jul 25, 2022

wfurt left a comment

antonfirsov commented Aug 3, 2022

wfurt commented Aug 4, 2022

tmds commented Aug 5, 2022

	// For that we the base-SafeHandle needs to be owning even
	// For that the base-SafeHandle needs to be owning even

		SocketError errorCode = OwnsHandle ? DoCloseHandle(abortive) : SocketError.Success;
		return ret = errorCode == SocketError.Success;

	SocketError errorCode = OwnsHandle ? DoCloseHandle(abortive) : SocketError.Success;
	return ret = errorCode == SocketError.Success;
	return !OwnsHandle \|\| DoCloseHandle(abortive) == SocketError.Success;

Socket: cancel on-going operations when Sockets with non-owning handle gets disposed. #72000

Socket: cancel on-going operations when Sockets with non-owning handle gets disposed. #72000

Conversation

tmds commented Jul 12, 2022

ghost commented Jul 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antonfirsov Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfurt left a comment

Choose a reason for hiding this comment

antonfirsov commented Aug 3, 2022

wfurt commented Aug 4, 2022

tmds commented Aug 5, 2022

antonfirsov Jul 18, 2022 •

edited

Loading