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

SocketAsyncContext.Unix: fix double processing of AsyncOperations #45683

Merged
merged 2 commits into from
Dec 15, 2020

Conversation

tmds
Copy link
Member

@tmds tmds commented Dec 7, 2020

When a socket event occurs, the pending operations gets triggered
to continue their work by calling the Process method.

The changes in #37974
cause Process to be called twice on the same AsyncOperation.

When Process is called, the operation can complete, and the
AsyncOperation instance may be reused for a different operation.

Fixes #45673

cc @geoffkizer @karelz @antonfirsov @stephentoub @dotnet/ncl

@ghost
Copy link

ghost commented Dec 7, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

When a socket event occurs, the pending operations gets triggered
to continue their work by calling the Process method.

The changes in #37974
cause Process to be called twice on the same AsyncOperation.

When Process is called, the operation can complete, and the
AsyncOperation instance may be reused for a different operation.

Fixes #45673

cc @geoffkizer @karelz @antonfirsov @stephentoub @dotnet/ncl

Author: tmds
Assignees: -
Labels:

area-System.Net.Sockets

Milestone: -

@stephentoub
Copy link
Member

Thanks. We need to service for this as well, right? Seems like this could lead to really inconsistent and bad behaviors?

@tmds
Copy link
Member Author

tmds commented Dec 7, 2020

Thanks. We need to service for this as well, right? Seems like this could lead to really inconsistent and bad behaviors?

Yes, I also think we need to service this.
In some cases the locks and state prevent this from becoming a problem, but that's not true in general.

@geoffkizer I'm glad you noticed this. Figuring it out from a product bug report would have been a real challenge.

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@geoffkizer
Copy link
Contributor

As a minimal fix this seems fine.

That said, I don't think we need the processAsyncEvents parameter at all. Instead we could just return the operation to the caller to decide how to process it. That seems like it would eliminate the confusion over who should process the event -- it should always be the caller, for async events.

When a socket event occurs, the pending operations gets triggered
to continue their work by calling the Process method.

The changes in dotnet#37974
cause Process to be called twice on the same AsyncOperation.

When Process is called, the operation can complete, and the
AsyncOperation instance may be reused for a different operation.
@karelz
Copy link
Member

karelz commented Dec 15, 2020

@geoffkizer can you please review the updated changes?

@geoffkizer geoffkizer merged commit 87ff54b into dotnet:master Dec 15, 2020
@geoffkizer
Copy link
Contributor

@tmds To clarify the impact of this issue...

I don't think an operation will ever be processed twice because of this issue, right? The logic in ProcessSyncEventOrGetAsyncEvent will return null if the op is processed, so the caller will not process it again.

What will happen though is that the operation is always processed directly on the calling thread, as opposed to the logic in HandleEvents where we will dispatch one of the operations to the thread pool for processing if we have two operations to process. This seems like enough of a reason to backport it, as it could cause unexpected and mysterious delays in processing the second operation. But I don't think it's any worse than that, is it?

@tmds
Copy link
Member Author

tmds commented Dec 15, 2020

@geoffkizer that's right, and it's a lot less worse than I thought.

antonfirsov added a commit to antonfirsov/runtime that referenced this pull request Jan 8, 2021
…tnet#45683)

* SocketAsyncContext.Unix: fix double processing of AsyncOperations

When a socket event occurs, the pending operations gets triggered
to continue their work by calling the Process method.

The changes in dotnet#37974
cause Process to be called twice on the same AsyncOperation.

When Process is called, the operation can complete, and the
AsyncOperation instance may be reused for a different operation.

* Remove processAsyncEvents
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2021
@karelz karelz added this to the 6.0.0 milestone Jan 26, 2021
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.

[.NET 5 regression] Socket.AsyncOperation gets processed too many times
6 participants