Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

[release/2.1] Fix Sockets hang caused by concurrent Socket disposal (#29786) #29846

Merged
merged 1 commit into from
May 30, 2018
Merged

[release/2.1] Fix Sockets hang caused by concurrent Socket disposal (#29786) #29846

merged 1 commit into from
May 30, 2018

Conversation

stephentoub
Copy link
Member

@stephentoub
Fix Sockets hang caused by concurrent Socket disposal (dotnet#29786)
9604928 
* Fix Sockets hang caused by concurrent Socket disposal

On Windows, if Socket.Send/ReceiveAsync is used, and there's a race condition where the Socket is disposed of between the time that the Socket checks for disposal and attempts to get a NativeOverlapped for use with the operation, the getting of the NativeOverlapped can throw an exception and cause us to leave a field in an inconsistent state, which in turn can cause the operation to hang, waiting for that state to be reset.

The fix is to correct the order in which we get the NativeOverlapped vs setting the relevant field.

This is a regression in 2.1.

* Change ArgumentException to ObjectDisposedException from concurrent disposal

If Socket is disposed of concurrently with the first operation on it that allocates a native overlapped, if the disposal happens between the check for disposal and the call to BindHandle, BindHandle may end up throwing an ArgumentException, which is not expected out of SendAsync.  We should instead throw an ObjectDisposedException.

* Add test for concurrent Send/ReceiveAsync and Dispose on Socket
@stephentoub stephentoub added this to the 2.1.x milestone May 22, 2018
@danmoseley danmoseley requested a review from davidsh May 29, 2018 23:45
@danmoseley
Copy link
Member

@davidsh the branch is open and this is approved, could you please do the honors of a CR of this port?

@danmoseley
Copy link
Member

@davidsh failures are known issues so if you sign off, you can merge this.

@davidsh
Copy link
Contributor

davidsh commented May 30, 2018 •
edited
Loading

@davidsh failures are known issues so if you sign off, you can merge this.

I noticed that no Outerloop tests were run. I know that the newly added tests are Innerloop. But we usually run Outerloop tests especially when networking product code is changed. Since this is a Windows-only change to Sockets, I'll run just the Windows Outerloop.

@davidsh
Copy link
Contributor

davidsh commented May 30, 2018

@dotnet-bot test Outerloop Windows x64 Debug Build

davidsh
davidsh approved these changes May 30, 2018
View reviewed changes
Copy link
Contributor

@davidsh davidsh left a comment

Choose a reason for hiding this comment

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

LGTM. Ok to merge after Outerloop CI run.

@danmoseley
Copy link
Member

Outerloop failure is a hang in a System.Runtime test. Stack dumping failed. Unlikely to be related but let's rerun.

@dotnet-bot test Outerloop Windows x64 Debug Build please

@davidsh
Copy link
Contributor

davidsh commented May 30, 2018

The only Sockets failures are in Nano which is expected (those multicast tests were recently disabled on Nano in master branch).

@danmosemsft Please confirm the other outerloop failures are expected (I don't think they are related to this PR). If so, this is ok to merge.

@danmoseley
Copy link
Member

Windows issues are known/irrelevant.

@danmoseley danmoseley merged commit 7ce9270 into dotnet:release/2.1 May 30, 2018
@ahsonkhan
Copy link

FYI, we were able to get dumps for the System.Runtime test hang (it is this test: System.Runtime.ExceptionServices.Tests.HandleProcessCorruptedStateExceptionsTests.ProcessExit_Called).

https://dumpling-internal.int-dot.net/api/dumplings/archived/14a9341455838a53d7c662bef31bd4f991a03c0f

 Child SP               IP Call Site
0000003cdbc7ca68 00007fffb9fb09d4 [HelperMethodFrame_1OBJ: 0000003cdbc7ca68] System.Threading.WaitHandle.WaitOneNative(System.Runtime.InteropServices.SafeHandle, UInt32, Boolean, Boolean)
0000003cdbc7cb80 00007fff8a6d6d7b System.Threading.WaitHandle.InternalWaitOne(System.Runtime.InteropServices.SafeHandle, Int64, Boolean, Boolean) [E:\A\_work\31\s\src\mscorlib\src\System\Threading\WaitHandle.cs @ 199]
0000003cdbc7cbb0 00007fff36b42197 System.Diagnostics.Process.WaitForExitCore(Int32)
0000003cdbc7cc50 00007fff36b41f91 System.Diagnostics.Process.WaitForExit(Int32)
0000003cdbc7ccb0 00007fff3702819b System.Diagnostics.Process.WaitForExit()
0000003cdbc7ccf0 00007fff37027f7d System.Runtime.ExceptionServices.Tests.HandleProcessCorruptedStateExceptionsTests.ProcessExit_Called()
0000003cdbc7cf88 00007fff9613f3b3 [DebuggerU2MCatchHandlerFrame: 0000003cdbc7cf88] 
0000003cdbc7d328 00007fff9613f3b3 [HelperMethodFrame_PROTECTOBJ: 0000003cdbc7d328] System.RuntimeMethodHandle.InvokeMethod(System.Object, System.Object[], System.Signature, Boolean, Boolean)
0000003cdbc7d470 00007fff8a64e3b1 System.Reflection.RuntimeMethodInfo.Invoke(System.Object, System.Reflection.BindingFlags, System.Reflection.Binder, System.Object[], System.Globalization.CultureInfo) [E:\A\_work\31\s\src\mscorlib\src\System\Reflection\RuntimeMethodInfo.cs @ 472]
0000003cdbc7d4e0 00007fff3699860b Xunit.Sdk.TestInvoker`1[[System.__Canon, System.Private.CoreLib]].CallTestMethod(System.Object) [C:\BuildAgent\work\cb37e9acf085d108\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs @ 147]
0000003cdbc7d520 00007fff36998323 Xunit.Sdk.TestInvoker`1+c__DisplayClass46_1+<b__1>d[[System.__Canon, System.Private.CoreLib]].MoveNext() [C:\BuildAgent\work\cb37e9acf085d108\src\xunit.execution\Sdk\Frameworks\Runners\TestInvoker.cs @ 224]
0000003cdbc7d5b0 00007fff36997bdf System.Runtime.CompilerServices.AsyncMethodBuilderCore.Start[[Xunit.Sdk.TestInvoker`1+c__DisplayClass46_1+<b__1>d[[System.__Canon, System.Private.CoreLib]], xunit.execution.dotnet]](<b__1>d<System.__Canon> ByRef) [E:\A\_work\31\s\src\mscorlib\src\System\Runtime\CompilerServices\AsyncMethodBuilder.cs @ 958]
0000003cdbc7d630 00007fff36997ae7 Xunit.Sdk.TestInvoker`1+c__DisplayClass46_1[[System.__Canon, System.Private.CoreLib]].b__1()
...

@stephentoub stephentoub deleted the port29786 branch July 4, 2018 04:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@davidsh davidsh davidsh approved these changes

Assignees

@stephentoub stephentoub

Labels
area-System.Net.Sockets
Projects
None yet
Milestone
2.1.x
Development

Successfully merging this pull request may close these issues.
4 participants
@stephentoub @danmoseley @davidsh @ahsonkhan