Reduce execution time of Socket handle inheritance tests #31813
Conversation
davidsh
added
area-System.Net.Sockets
test-enhancement
src/libraries/Common/tests/System/Net/Sockets/SocketTestExtensions.cs
Outdated
Show resolved
Hide resolved
| // Useful to speed up "can not connect" assertions on Windows | ||
| public static bool TryConnect(this Socket socket, EndPoint remoteEndpoint, int millisecondsTimeout) | ||
| { | ||
| using var mre = new ManualResetEvent(false); |
There was a problem hiding this comment.
Nit: doesn't matter as much for tests, but it'd be cheaper to use ManualResetEventSlim
There was a problem hiding this comment.
Probably better not to dispose of this. You currently have a race condition where you could dispose of it and then set it.
There was a problem hiding this comment.
Re MRE:
Initially I wanted to use the slim variant, but I was afraid that the spinning could make things worse. Isn't 300ms too long for ManualResetEventSlim? Any resources / guidance about how to choose?
Re Dispose:
See my next comment.
There was a problem hiding this comment.
I was afraid that the spinning could make things worse. Isn't 300ms too long for ManualResetEventSlim?
MRES doesn't just spin; after a tiny amount of spinning it'll fall back to an actual kernel wait.
If you're actually concerned about it (and I don't think you need to be), you can pass spinCount: 0 to the ctor.
| return sea.SocketError == SocketError.Success; | ||
| } | ||
|
|
||
| sea.UserToken = null; // Letting the event handler know that we timed out |
There was a problem hiding this comment.
Not really necessary; you could have the event handler always set it and not null it out. Then you can remove this like and simplify the handler to just:
sea.Completed += (s, e) => ((ManualResetEventSlim)e.UserToken).Set();There was a problem hiding this comment.
I'm doing this to address the race condition you pointed out in your previous comment. Without this, a pending operation may try to use a disposed MRE.
With the current logic, we are safe unless an unexpected exception is thrown.
There was a problem hiding this comment.
Ok you are right, I still have a race around the if in the handler, but it seems unlikely to cause trouble in test code.
Is it indeed safer to just leave the MRE undisposed instead?
There was a problem hiding this comment.
it seems unlikely to cause trouble in test code.
I can almost guarantee it would happen at some point in CI and cause a spurious test failure.
Is it indeed safer to just leave the MRE undisposed instead?
Yes
src/libraries/System.Net.Sockets/tests/FunctionalTests/CreateSocketTests.cs
Show resolved
Hide resolved
|
@stephentoub @wfurt anything against merging this as it is now? |
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
No tests failures seem to be related with the changes in this PR. |
ghost
locked as resolved and limited conversation to collaborators
Follow up for the discussion under #1858 (comment)
This PR reduces the execution time for
CtorAndAccept_SocketNotKeptAliveViaInheritanceandDuplicateSocket_IsNotInheritablefrom2sec+to~800mson Windows.The tests were running long because of the long time
WSAConnectkeeps blocking the caller before failing.