Socket.Select: increase ref count while the handle is in use #41763

tmds · 2019-10-14T13:05:08Z

CC @stephentoub

wfurt · 2019-10-14T18:56:57Z

why do we need this @tmds? Should SafeHandle be sufficient to keep it alive?

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

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

src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs

stephentoub · 2019-10-14T19:21:51Z

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

                {
                    throw new ArgumentException(SR.Format(SR.net_sockets_select, socketList[current].GetType().FullName, typeof(System.Net.Sockets.Socket).FullName), nameof(socketList));
                }

-                fileDescriptorSet[current + 1] = ((Socket)socketList[current])._handle.DangerousGetHandle();
+                bool success = false;
+                socket.SafeHandle.DangerousAddRef(ref success);


With your other PR, accessing socket.SafeHandle is now going to mark the handle as exposed. Is that necessary? Should we instead be using _handle here and everywhere else internally where the handle is used?

I've just added an InternalSafeHandle property to the other PR. Once it is merged, I'll rebase and use it here.

stephentoub · 2019-10-14T19:23:54Z

why do we need this @tmds? Should SafeHandle be sufficient to keep it alive?

The problem is the implementation isn't just passing SafeHandle to P/Invokes, where the marshaling layer would implicitly addref and release around the native call. Rather, the implementation is calling DangerousGetHandle, returning the raw pointer to then be used. Any usage of that raw handle is no longer guarded by the SafeHandle; instead, the SafeHandle needs to have its ref count increased prior to any usage of the raw handle and its ref count then decremented after. Otherwise, Dispose'ing of the SafeHandle could cause the handle to be closed and even recycled while it's still in use.

wfurt · 2019-10-14T19:36:29Z

why do we need this @tmds? Should SafeHandle be sufficient to keep it alive?

The problem is the implementation isn't just passing SafeHandle to P/Invokes, where the marshaling layer would implicitly addref and release around the native call. Rather, the implementation is calling DangerousGetHandle, returning the raw pointer to then be used. Any usage of that raw handle is no longer guarded by the SafeHandle; instead, the SafeHandle needs to have its ref count increased prior to any usage of the raw handle and its ref count then decremented after. Otherwise, Dispose'ing of the SafeHandle could cause the handle to be closed and even recycled while it's still in use.

I was under impression that was fixed in 3.0. If not, shoud we pass SafeHandle to the call?
I did not see any linked issues so I'm trying understand where this come form.

stephentoub · 2019-10-14T19:38:11Z

shoud we pass SafeHandle to the call?

We need to be able to pass a variable number of handles / file descriptors in an array; we can't do that with SafeHandle.

I was under impression that was fixed in 3.0.

What specifically?

tmds · 2019-10-15T08:39:47Z

I was under impression that was fixed in 3.0.

For the operations that take a single handle, the raw values were replaced by SafeHandle marshalling as part of #38804. Socket.Select wasn't changed yet.

stephentoub · 2019-10-21T02:48:41Z

The failures here (at least some of them) appear to be related hangs:

[Long Running Test] 'System.Net.Sockets.Tests.AcceptSync.AcceptGetsCanceledByDispose', Elapsed: 00:14:36
[Long Running Test] 'System.Net.Sockets.Tests.ConnectSync.ConnectGetsCanceledByDispose', Elapsed: 00:14:34
[Long Running Test] 'System.Net.Sockets.Tests.SocketOptionNameTest.MulticastInterface_Set_Loopback_Succeeds', Elapsed: 00:14:36
[Long Running Test] 'System.Net.Sockets.Tests.SendReceiveSpanSync.TcpPeerReceivesFinOnShutdownWithPendingData', Elapsed: 00:14:36
[Long Running Test] 'System.Net.Sockets.Tests.SendReceiveEap.SendAsync_ConcurrentDispose_SucceedsOrThrowsAppropriateException', Elapsed: 00:14:32
[Long Running Test] 'System.Net.Sockets.Tests.ConnectApm.ConnectGetsCanceledByDispose', Elapsed: 00:14:34
[Long Running Test] 'System.Net.Sockets.Tests.DualModeBeginConnectToIPAddress.BeginConnectV4IPAddressToV6Host_Fails', Elapsed: 00:14:24
[Long Running Test] 'System.Net.Sockets.Tests.AcceptSyncForceNonBlocking.AcceptGetsCanceledByDispose', Elapsed: 00:14:32
[Long Running Test] 'System.Net.Sockets.Tests.SendReceiveTask.SendAsync_ConcurrentDispose_SucceedsOrThrowsAppropriateException', Elapsed: 00:14:32
[Long Running Test] 'System.Net.Sockets.Tests.DualModeBeginConnect.DualModeBeginConnect_DnsEndPointToHost_Helper', Elapsed: 00:14:32
[Long Running Test] 'System.Net.Sockets.Tests.ConnectSyncForceNonBlocking.ConnectGetsCanceledByDispose', Elapsed: 00:14:24
[Long Running Test] 'System.Net.Sockets.Tests.DisposedSocket.NonDisposedSocket_SafeHandlesCollected', Elapsed: 00:14:24
[Long Running Test] 'System.Net.Sockets.Tests.AcceptTask.AcceptAsync_MultipleAcceptsThenDispose_AcceptsThrowAfterDispose', Elapsed: 00:14:24
[Long Running Test] 'System.Net.Sockets.Tests.ConnectEap.ConnectGetsCanceledByDispose', Elapsed: 00:14:24
[Long Running Test] 'System.Net.Sockets.Tests.DualModeBeginConnectToIPEndPoint.BeginConnectV6IPEndPointToV6Host_Success', Elapsed: 00:14:18
[Long Running Test] 'System.Net.Sockets.Tests.AcceptEap.AcceptGetsCanceledByDispose', Elapsed: 00:14:24
[Long Running Test] 'System.Net.Sockets.Tests.SelectAndPollTests.SelectRead_Single_Timeout', Elapsed: 00:14:10
[Long Running Test] 'System.Net.Sockets.Tests.SendReceiveSpanSyncForceNonBlocking.Receive0ByteReturns_WhenPeerDisconnects', Elapsed: 00:14:18
[Long Running Test] 'System.Net.Sockets.Tests.ConnectTask.Connect_OnConnectedSocket_Fails', Elapsed: 00:14:18
[Long Running Test] 'System.Net.Sockets.Tests.ArgumentValidation.Socket_Connect_DnsEndPointWithIPAddressString_Supported', Elapsed: 00:14:18
[Long Running Test] 'System.Net.Sockets.Tests.SelectTest.Select_ReadError_NoneReady', Elapsed: 00:14:18

stephentoub · 2019-10-21T02:49:02Z

/azp run corefx-ci

azure-pipelines · 2019-10-21T02:49:13Z

Azure Pipelines successfully started running 1 pipeline(s).

tmds · 2019-10-22T11:51:54Z

On Windows, there were missing unrefs. This is fixed and ci is now passing.
@stephentoub can you give it another run, to check if everything stays green?

stephentoub · 2019-10-22T19:33:33Z

/azp run

azure-pipelines · 2019-10-22T19:33:54Z

Azure Pipelines successfully started running 4 pipeline(s).

stephentoub

Thanks.

…corefx#41763) * Socket.Select: increase ref count while the handle is in use * PR feedback * Use Socket.InternalSafeHandle for ref/release. * Socket.Windows: SelectFileDescriptor: add missing unrefs Commit migrated from dotnet/corefx@6c62c7f

Dotnet-GitSync-Bot added the area-System.Net.Sockets label Oct 14, 2019

davidsh added this to the 5.0 milestone Oct 14, 2019

davidsh requested a review from a team October 14, 2019 16:50

stephentoub reviewed Oct 14, 2019

View reviewed changes

src/System.Net.Sockets/src/System/Net/Sockets/Socket.Windows.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 14, 2019

View reviewed changes

src/System.Net.Sockets/src/System/Net/Sockets/Socket.cs Show resolved Hide resolved

stephentoub reviewed Oct 14, 2019

View reviewed changes

src/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs Outdated Show resolved Hide resolved

stephentoub reviewed Oct 14, 2019

View reviewed changes

tmds added 2 commits October 16, 2019 15:43

Socket.Select: increase ref count while the handle is in use

c3b26cf

PR feedback

a71657e

tmds force-pushed the select_safehandle branch from 72c6bbd to f993aab Compare October 16, 2019 14:01

Use Socket.InternalSafeHandle for ref/release.

328d455

tmds force-pushed the select_safehandle branch from f993aab to 328d455 Compare October 16, 2019 14:05

Socket.Windows: SelectFileDescriptor: add missing unrefs

d9b4789

stephentoub approved these changes Oct 23, 2019

View reviewed changes

stephentoub merged commit 6c62c7f into dotnet:master Oct 23, 2019

stephentoub mentioned this pull request Oct 23, 2019

Add test for SafeHandle handling in Socket.Select #42018

Merged

tmds mentioned this pull request Apr 13, 2020

Socket.Select throws ObjectDisposedException when one of the sockets is disposed dotnet/runtime#34886

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Socket.Select: increase ref count while the handle is in use #41763

Socket.Select: increase ref count while the handle is in use #41763

tmds commented Oct 14, 2019

wfurt commented Oct 14, 2019

stephentoub Oct 14, 2019

tmds Oct 15, 2019

stephentoub commented Oct 14, 2019

wfurt commented Oct 14, 2019

stephentoub commented Oct 14, 2019

tmds commented Oct 15, 2019

stephentoub commented Oct 21, 2019 •

edited

Loading

stephentoub commented Oct 21, 2019

azure-pipelines bot commented Oct 21, 2019

tmds commented Oct 22, 2019 •

edited

Loading

stephentoub commented Oct 22, 2019

azure-pipelines bot commented Oct 22, 2019

stephentoub left a comment

Socket.Select: increase ref count while the handle is in use #41763

Socket.Select: increase ref count while the handle is in use #41763

Conversation

tmds commented Oct 14, 2019

wfurt commented Oct 14, 2019

stephentoub Oct 14, 2019

Choose a reason for hiding this comment

tmds Oct 15, 2019

Choose a reason for hiding this comment

stephentoub commented Oct 14, 2019

wfurt commented Oct 14, 2019

stephentoub commented Oct 14, 2019

tmds commented Oct 15, 2019

stephentoub commented Oct 21, 2019 • edited Loading

stephentoub commented Oct 21, 2019

azure-pipelines bot commented Oct 21, 2019

tmds commented Oct 22, 2019 • edited Loading

stephentoub commented Oct 22, 2019

azure-pipelines bot commented Oct 22, 2019

stephentoub left a comment

Choose a reason for hiding this comment

stephentoub commented Oct 21, 2019 •

edited

Loading

tmds commented Oct 22, 2019 •

edited

Loading