-
Notifications
You must be signed in to change notification settings - Fork 520
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
[Foundation] Break out of the delegate when we have canceled a task. … #10930
Merged
mandel-macaque
merged 1 commit into
dotnet:xcode12.5
from
mandel-macaque:backport-nsurlsession
Mar 22, 2021
Merged
[Foundation] Break out of the delegate when we have canceled a task. … #10930
mandel-macaque
merged 1 commit into
dotnet:xcode12.5
from
mandel-macaque:backport-nsurlsession
Mar 22, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ixes dotnet#9132 (dotnet#10230) This is an interesting issue that happens in the following scenario: 1. A request is performed that will take some extra time to get a response or an error. 2. The request is cancelled BEFORE the delegate methods DidReceiveResponse, DidReceiveData or DidCompleteWithError and called. Yet we have not yet fully canceled the native request. It does take some time to do so, that is why we have a 'NSURLSessionTaskStateCanceling' and not 'NSURLSessionTaskStateCancelled' The issue happens because the GetInflightData checks if the task has been canceled. In that method we have the following: ```csharp if (inflight.CancellationToken.IsCancellationRequested) task?.Cancel (); return inflight; ``` The call of the Cancel method in the NSData task has as ripple effect which is the execution of the following callback: ```csharp cancellationToken.Register (() => { RemoveInflightData (dataTask); tcs.TrySetCanceled (); }); ``` Which calls: ```csharp void RemoveInflightData (NSUrlSessionTask task, bool cancel = true) { lock (inflightRequestsLock) { if (inflightRequests.TryGetValue (task, out var data)) { if (cancel) data.CancellationTokenSource.Cancel (); data.Dispose (); inflightRequests.Remove (task); } // do we need to be notified? If we have not inflightData, we do not if (inflightRequests.Count == 0) RemoveNotification (); } if (cancel) task?.Cancel (); task?.Dispose (); } ``` The above call does call Dispose and Dipose in the inflight data does: ```csharp if (disposing) { if (CancellationTokenSource != null) { CancellationTokenSource.Dispose (); CancellationTokenSource = null; } } ``` If we follow these set of events for example in the DidRecieveResponse: ```chsarp [Preserve (Conditional = true)] public override void DidReceiveResponse (NSUrlSession session, NSUrlSessionDataTask dataTask, NSUrlResponse response, Action<NSUrlSessionResponseDisposition> completionHandler) { var inflight = GetInflightData (dataTask); if (inflight == null) return; try { var urlResponse = (NSHttpUrlResponse)response; var status = (int)urlResponse.StatusCode; var content = new NSUrlSessionDataTaskStreamContent (inflight.Stream, () => { if (!inflight.Completed) { dataTask.Cancel (); } inflight.Disposed = true; inflight.Stream.TrySetException (new ObjectDisposedException ("The content stream was disposed.")); sessionHandler.RemoveInflightData (dataTask); }, inflight.CancellationTokenSource.Token); ``` If can happen that, when we get the delegate call to the method, the request has been cancelled. That means that we are in the precise state in which we do not longer care about the response BUT we did get a infligh data reference.. that HAS BEEN DIPOSED!!! this later gives a NRE in several places. The correct way is to return null from the GetInflighData method if we have been cancelled, this makes sense because if we cancelled we do not care about the execution of any of the methods that required the data since it has been disposed! Co-authored-by: Rolf Bjarne Kvinge <[email protected]>
Manual backport of #9132 requested by @dalexsoto |
spouliot
approved these changes
Mar 22, 2021
dalexsoto
approved these changes
Mar 22, 2021
rolfbjarne
approved these changes
Mar 22, 2021
❌ Tests failed on Build ❌Tests failed on Build. API diff✅ API Diff from stable Test results1 tests failed, 174 tests passed.Failed tests
Pipeline on Agent XAMBOT-1109 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…Fixes #9132 (#10230)
This is an interesting issue that happens in the following scenario:
response or an error.
DidReceiveResponse, DidReceiveData or DidCompleteWithError and
called. Yet we have not yet fully canceled the native request. It
does take some time to do so, that is why we have a
'NSURLSessionTaskStateCanceling' and not 'NSURLSessionTaskStateCancelled'
The issue happens because the GetInflightData checks if the task has
been canceled. In that method we have the following:
The call of the Cancel method in the NSData task has as ripple effect which
is the execution of the following callback:
Which calls:
The above call does call Dispose and Dipose in the inflight data does:
If we follow these set of events for example in the
DidRecieveResponse:
If can happen that, when we get the delegate call to the method, the
request has been cancelled. That means that we are in the precise state
in which we do not longer care about the response BUT we did get a
infligh data reference.. that HAS BEEN DIPOSED!!! this later gives a NRE
in several places.
The correct way is to return null from the GetInflighData method if we
have been cancelled, this makes sense because if we cancelled we do not
care about the execution of any of the methods that required the data
since it has been disposed!
Co-authored-by: Rolf Bjarne Kvinge [email protected]