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

[Foundation] Break out of the delegate when we have canceled a task. … #10930

Merged

Conversation

mandel-macaque
Copy link
Member

Fixes #9132 (#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:

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:

cancellationToken.Register (() => {
  RemoveInflightData (dataTask);
  tcs.TrySetCanceled ();
});

Which calls:

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:

if (disposing) {
    if (CancellationTokenSource != null) {
            CancellationTokenSource.Dispose ();
            CancellationTokenSource = null;
    }
}

If we follow these set of events for example in the
DidRecieveResponse:

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

…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]>
@mandel-macaque mandel-macaque added backported note-highlight Worth calling out specifically in release notes labels Mar 22, 2021
@mandel-macaque
Copy link
Member Author

Manual backport of #9132 requested by @dalexsoto

@vs-mobiletools-engineering-service2
Copy link
Collaborator

❌ Tests failed on Build ❌

Tests failed on Build.

API diff

✅ API Diff from stable

View API diff

Test results

1 tests failed, 174 tests passed.

Failed tests

  • introspection/watchOS 32-bits - simulator/Debug (watchOS 5.0): LaunchFailure

Pipeline on Agent XAMBOT-1109

@mandel-macaque mandel-macaque merged commit f969e83 into dotnet:xcode12.5 Mar 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported note-highlight Worth calling out specifically in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants