Remove SocketAsyncEngine.Token#36339
Conversation
This removes SocketAsyncEngine.Token. The token ensured each socket has a unique key for receiving events on a poll thread. These unique tokens were introduced to deal with fd recyling leading to false events being delivered to the new socket (when the fd is used as the key). This is not an issue for most operations because they can be retried. It is an issue specifically for the connect operation which is an on-going operation for which the status must be read using SO_ERROR socket option when the socket becomes writable. The connect-case is solved by checking the socket is writable before reading the SO_ERROR socket option. By using the fd as handles we can remove the logic for generated unique tokens, which includes spinning up additional poll threads in case unique handles are exhausted.
|
Tagging subscribers to this area: @dotnet/ncl |
|
@adamsitnik this is an extended version of #36115 cc @stephentoub |
|
I've run all 3 Platform benchmarks for this change and verified that there is no regression (and also no improvement). |
|
Thanks for verifying, Adam. The main value of this change is that it eliminates a code path that is used only very rarely. |
I know and I like it a LOT! |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
Outdated
Show resolved
Hide resolved
|
Conflicts, rebase? |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
Show resolved
Hide resolved
|
@antonfirsov maybe you want to review this? |
antonfirsov
left a comment
There was a problem hiding this comment.
Mostly looks good, not sure if my questions are valid, or I'm just missing things.
If possible, I would try to add a few new tests to ensure correctness when registering / unregistering Sockets with SocketAsyncEngine concurrently.
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketPal.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
Show resolved
Hide resolved
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncEngine.Unix.cs
Show resolved
Hide resolved
|
/azp run runtime-libraries outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
CI fails seem unrelated, some tests failed on Windows. |
src/libraries/System.Net.Sockets/src/System/Net/Sockets/SocketAsyncContext.Unix.cs
Show resolved
Hide resolved
|
This is a nice change, thanks for making this happen. |
This removes SocketAsyncEngine.Token. The token ensured each socket
has a unique key for receiving events on a poll thread.
These unique tokens were introduced to deal with fd recyling leading
to false events being delivered to the new socket (when the fd is used
as the key).
This is not an issue for most operations because they can be retried.
It is an issue specifically for the connect operation which is an
on-going operation for which the status must be read using SO_ERROR
socket option when the socket becomes writable.
The connect-case is solved by checking the socket is writable
before reading the SO_ERROR socket option.
By using the fd as handles we can remove the logic for generated
unique tokens, which includes spinning up additional poll threads
in case unique handles are exhausted.