-
Notifications
You must be signed in to change notification settings - Fork 523
Conversation
@@ -248,8 +245,7 @@ public void Abort() | |||
ResponseBody = _responseBody; | |||
DuplexStream = new FrameDuplexStream(RequestBody, ResponseBody); | |||
|
|||
_requestAbortCts = CancellationTokenSource.CreateLinkedTokenSource(_disconnectCts.Token); | |||
RequestAborted = _requestAbortCts.Token; | |||
RequestAborted = _disconnectOrAbortedCts.Token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is incorrect. RequestAborted needs to be unique per request, not per connection. We don't want registrations from one request to fire for the next request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the behaviour is after the current request finishes it is Disposed; and if you actually call Abort()
it aborts and disconnects?
Will change to maintain that behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a successful request the token should be disposed without triggering to discard all the registrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually does it do anything particularly? Its only activated on Abort
but that also disconnects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should activate for disconnects and Abort, and Abort also disconnects. It tells the application to abandon any processing and unwind the middleware chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
K, might be looking at this wrong. Does the _disconnectCts
do anything particularly beyond the _requestAbortCts
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benaadams It avoids extra synchronization logic that might be required to avoid canceling an already disposed _requestAbortCts
when called from places like Connection.Abort()
.
Turns out that there is still an issue with canceling a disposed _disconnectCts
, but at least that isn't being disposed after every request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switched it round?
That might work. @halter73? |
Need a |
@@ -199,12 +199,17 @@ public void Abort() | |||
ConnectionControl.End(ProduceEndType.SocketDisconnect); | |||
SocketInput.AbortAwaiting(); | |||
|
|||
_disconnectCts.Cancel(); | |||
_abortedCts?.Cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe wrap this call specifically since we're not locking to avoid ODEs (however unlikely). This prevents us from logging errors that we know can happen but the app developer can do nothing about.
try
{
_abortedCts.Cancel();
}
catch (ObjectDisposedException)
{
// Don't log ODEs thrown from _abortedCts.Cancel()
// If _disconnectCts is disposed, the app has already completed.
}
If Connection.Abort was called between requests and an app registered a callback with the RequestAborted token on the next request, the callback would be triggered immediately. In the end, I don't think this matters since both the request and response are poisoned. Just thought it would be worth mentioning that the behavior changes slightly. |
The next request and response streams aren't poisoned I guess, but we still close the socket and poison SocketInput so I still think we're OK. |
Isn't that correct because that request if it managed to start would be correctly aborted? Or do you mean it should be receiving a different error? |
@benaadams That quote was me trying to describe the old behavior. I don't think the abort callback for the next request will be called after this change. |
Ah kk |
Avoid allocating the CancellationTokenSource unless it's actually requested. This makes it pay-for-play with regards to code that actually asks for the RequestAborted token and requests that are aborted.
Lazily allocate the RequestAborted CTS
@stephentoub's added changes completely clear up the allocations |
@@ -146,7 +185,8 @@ public void Reset() | |||
|
|||
_prepareRequest?.Invoke(this); | |||
|
|||
_requestAbortCts?.Dispose(); | |||
_manuallySetRequestAbortToken = null; | |||
_abortedCts = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need to to dispose the last _abortedCts (if it exists) to clean up registrations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @stephentoub
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. CancellationTokenSource.Dispose does a few things:
- Disposes of the ManualResetEvent that's lazily-allocated when the WaitHandle is accessed
- Disposes of the Timer that's created if the CTS's time-based ctor or its CancelAfter method is used
- Disposes of the linked registrations created from a CTS created with CreateLinkedTokenSource
The third one isn't relevant as this CTS isn't being created with CreateLinkedTokenSource.
The second one isn't relevant as we're not using timers.
And while the third one is possibly relevant, it's a) rare that code these days uses CancellationToken.WaitHandle, and b) if it does get lazily-allocated, it'll still get cleaned up via the finalizer.
Given all that, it didn't seem worthwhile to Dispose of it and have to have other code that worried about dispose with the resulting race conditions on its use.
(Dispose also nulls out some fields, but as we're nulling out the reference to the CTS, the whole thing will become eliglble for GC, anyway. The null'ing out is only relevant if the CTS itself is being rooted.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stephentoub Thanks for the detailed explanation.
{ | ||
// If a request abort token was previously explicitly set, return it. | ||
if (_manuallySetRequestAbortToken.HasValue) | ||
return _manuallySetRequestAbortToken.Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: style, always use braces
Remove CreateLinkedTokenSource
Resolves #407