[Proposal] remove lock from SocketAsyncEngine

[adamsitnik]

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]

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

[stephentoub]

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]

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.

[tmds]

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

[adamsitnik]

@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]

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.

[tmds]

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).

[adamsitnik]

@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)