-
Notifications
You must be signed in to change notification settings - Fork 5.4k
SocketAsyncContext.Unix: remove Lock from IsReady #36705
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
Changes from all commits
48a6f94
4604d72
b8bcfd0
bdb78f7
3b226e6
b436b1b
a06db65
4400920
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -710,7 +710,7 @@ private struct OperationQueue<TOperation> | |
| // If we successfully process all enqueued operations, then the state becomes Ready; | ||
| // otherwise, the state becomes Waiting and we wait for another epoll notification. | ||
|
|
||
| private enum QueueState : byte | ||
| private enum QueueState : int | ||
| { | ||
| Ready = 0, // Indicates that data MAY be available on the socket. | ||
| // Queue must be empty. | ||
|
|
@@ -753,18 +753,31 @@ public void Init() | |
| _sequenceNumber = 0; | ||
| } | ||
|
|
||
| // IsReady returns the current _sequenceNumber, which must be passed to StartAsyncOperation below. | ||
| // IsReady returns whether an operation can be executed immediately. | ||
| // observedSequenceNumber must be passed to StartAsyncOperation. | ||
| public bool IsReady(SocketAsyncContext context, out int observedSequenceNumber) | ||
| { | ||
| using (Lock()) | ||
| { | ||
| observedSequenceNumber = _sequenceNumber; | ||
| bool isReady = (_state == QueueState.Ready) || (_state == QueueState.Stopped); | ||
| // It is safe to read _state and _sequence without using Lock. | ||
| // - The return value is soley based on Volatile.Read of _state. | ||
| // - The Volatile.Read of _sequenceNumber ensures we read a value before executing the operation. | ||
| // This is needed to retry the operation in StartAsyncOperation in case the _sequenceNumber incremented. | ||
| // - Because no Lock is taken, it is possible we observe a sequence number increment before the state | ||
| // becomes Ready. When that happens, observedSequenceNumber is decremented, and StartAsyncOperation will | ||
| // execute the operation because the sequence number won't match. | ||
|
|
||
| Trace(context, $"{isReady}"); | ||
| Debug.Assert(sizeof(QueueState) == sizeof(int)); | ||
| QueueState state = (QueueState)Volatile.Read(ref Unsafe.As<QueueState, int>(ref _state)); | ||
tmds marked this conversation as resolved.
Show resolved
Hide resolved
tmds marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| observedSequenceNumber = Volatile.Read(ref _sequenceNumber); | ||
|
|
||
| return isReady; | ||
| bool isReady = state == QueueState.Ready || state == QueueState.Stopped; | ||
| if (!isReady) | ||
| { | ||
| observedSequenceNumber--; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there an enforced cap on the sequence number somewhere? I'm wondering what happens if, for example, this wraps around such that observedSequenceNumber is int.MinValue and observedSequenceNumber - 1 is int.MaxValue.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is no cap on sequence number. The observed sequence number gets checked for equality in
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I wasn't sure if it was ever compared with
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We'd not be aware events happened, and the socket operation will never complete. Since there is a limited nr of cases that cause increments, we don't need to worry about reaching 4B increments.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, not actually concerned, but trying to understand... re "Since there is a limited nr of cases that cause increments, we don't need to worry about reaching 4B increments", what would prevent one thread from stalling in between the get and the check, and other threads processing 4B operations in between? |
||
| } | ||
|
|
||
| Trace(context, $"{isReady}"); | ||
|
|
||
| return isReady; | ||
| } | ||
|
|
||
| // Return true for pending, false for completed synchronously (including failure and abort) | ||
|
|
@@ -1111,6 +1124,7 @@ public void CancelAndContinueProcessing(TOperation op) | |
| if (_tail == null) | ||
| { | ||
| _state = QueueState.Ready; | ||
| _sequenceNumber++; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change is unrelated. All code paths that set _state to QueueState.Ready increment _sequenceNumber. I'm not sure why it is not done at this location, so I've added it. |
||
| } | ||
| } | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.