Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Proposal] remove lock from SocketAsyncEngine #36115

Closed
wants to merge 6 commits into from

Conversation

adamsitnik
Copy link
Member

While working on #36019 I've noticed that we could simplify the SocketAsyncEngine by doing two things:

  • create the engines up-front and avoid locks when getting engine for a given context. It's a clear win for the majority of the cases (having a single engine), but an extra cost for scenarios with multiple (n) engines: the first async call is going to create n engines (and n epolls)
  • use existing socket file descriptors, don't generate our own ids using IntPtrs, and avoid lock when adding new context to the engine.

I don't know why we are generating new ids instead of using existing socket file descriptors (there was probably a good reason for that), so this is just a proposal.

/cc @tmds @stephentoub

@ghost
Copy link

ghost commented May 8, 2020

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

@stephentoub
Copy link
Member

I don't know why we are generating new ids instead of using existing socket file descriptors (there was probably a good reason for that), so this is just a proposal.

5e64fa3

@tmds
Copy link
Member

tmds commented May 8, 2020

I don't know why we are generating new ids instead of using existing socket file descriptors (there was probably a good reason for that), so this is just a proposal.

I was wondering about that too after #36019.

I was chatting with @stephentoub about it, and I think another way to resolve 5e64fa3 would be to check socket readability before reading SO_ERROR for connect completion.

@@ -289,6 +169,24 @@ private SocketAsyncEngine()
}
}

private void AddToMap(SocketAsyncContext context)
{
IntPtr socketFileDescriptor = context.GetSocketFileDescriptor();
Copy link
Member

Choose a reason for hiding this comment

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

This is using DangerousGetHandle which may cause issues (besides 5e64fa3) due to fd recycling.

@adamsitnik
Copy link
Member Author

@stephentoub @tmds thanks for providing the additional context.

This makes me wonder why simply don't remove given socket from epoll when it's being closed?

To my understanding we are just adding to epoll, never actually removing anything from it.

@tmds
Copy link
Member

tmds commented May 11, 2020

This makes me wonder why simply don't remove given socket from epoll when it's being closed?

That happens automatically in the kernel.

The race happens when the fd is already in the events array returned by epoll_wait. When we look it up in the ConcurrentDictionary, we don't know it was meant for the previous/current SocketAsyncContext that matches the fd.

error = Interop.Sys.TryChangeSocketEventRegistration(_port, socket,
Interop.Sys.SocketEvents.Read | Interop.Sys.SocketEvents.Write,
Interop.Sys.SocketEvents.None, socket.DangerousGetHandle());
return error == Interop.Error.SUCCESS;
Copy link
Member

Choose a reason for hiding this comment

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

This happens automagically in the kernel when the socket is closed. And it doesn't solve the race issue between the epoll thread and the thread that creates/closes sockets. See #36115 (comment).

…queue notification can be handled as soon as we receive it

if registration fails, remove context from the map

remove context from the map to make sure that we stop handling notifications first
@adamsitnik
Copy link
Member Author

@tmds thanks for all the great insights. I am going to close it for now and most probably send a simplified version later this week (just removing lock, not removing the id system)

@adamsitnik adamsitnik closed this May 11, 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
@adamsitnik adamsitnik deleted the socketAsyncEngineNoLock branch July 2, 2021 11:09
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.

5 participants