Skip to content

Remove SocketAsyncEngine.Token#36339

Merged
stephentoub merged 8 commits intodotnet:masterfrom
tmds:socket_token
Jun 11, 2020
Merged

Remove SocketAsyncEngine.Token#36339
stephentoub merged 8 commits intodotnet:masterfrom
tmds:socket_token

Conversation

@tmds
Copy link
Member

@tmds tmds commented May 13, 2020

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.

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.
@ghost
Copy link

ghost commented May 13, 2020

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

@tmds
Copy link
Member Author

tmds commented May 13, 2020

@adamsitnik this is an extended version of #36115

cc @stephentoub

@adamsitnik
Copy link
Member

I've run all 3 Platform benchmarks for this change and verified that there is no regression (and also no improvement).

@tmds
Copy link
Member Author

tmds commented May 18, 2020

Thanks for verifying, Adam.

The main value of this change is that it eliminates a code path that is used only very rarely.

@adamsitnik
Copy link
Member

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!

@benaadams
Copy link
Member

Conflicts, rebase?

@tmds
Copy link
Member Author

tmds commented May 29, 2020

@antonfirsov maybe you want to review this?

Copy link
Contributor

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

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.

@antonfirsov
Copy link
Contributor

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmds
Copy link
Member Author

tmds commented Jun 3, 2020

CI fails seem unrelated, some tests failed on Windows.

Copy link
Contributor

@antonfirsov antonfirsov left a comment

Choose a reason for hiding this comment

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

LGTM

@stephentoub stephentoub closed this Jun 9, 2020
@stephentoub stephentoub reopened this Jun 9, 2020
@geoffkizer
Copy link
Contributor

This is a nice change, thanks for making this happen.

@stephentoub stephentoub merged commit e77add9 into dotnet:master Jun 11, 2020
antonfirsov added a commit to antonfirsov/runtime that referenced this pull request Jun 12, 2020
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants