Skip to content

[Foundation] Break out of the delegate when we have canceled a task. Fixes #9132 - #10230

Merged
mandel-macaque merged 3 commits into
dotnet:mainfrom
mandel-macaque:nsurlsession-handler-crash
Jan 11, 2021
Merged

[Foundation] Break out of the delegate when we have canceled a task. Fixes #9132#10230
mandel-macaque merged 3 commits into
dotnet:mainfrom
mandel-macaque:nsurlsession-handler-crash

Conversation

@mandel-macaque

@mandel-macaque mandel-macaque commented Dec 7, 2020

Copy link
Copy Markdown
Contributor

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 DISPOSED!!! 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!

fixes #9132

…ixes dotnet#9132

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!
@mandel-macaque mandel-macaque added the notes-mention Deserves a mention in release notes label Dec 7, 2020

@spouliot spouliot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's get confirmation it fix the customers issues before backporting to release branches (since it looks unlikely to get a [unit] test for this)

//
// DidReceiveResponse We might have received a response, but either the user cancelled or a
// timeout did, if that is the case, we do not care about the respose.
// DidReceiveData Of buffer has a partial response ergo garbage and there is not real

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ergo ? we're now using latin for comments :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We are that posh :)

Comment thread src/Foundation/NSUrlSessionHandler.cs Outdated
// we did not find the data.
if (inflight.CancellationToken.IsCancellationRequested) {
task?.Cancel ();
// return null so that we break out of any deleate method.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

typo -> dele[g]ate

@mandel-macaque

Copy link
Copy Markdown
Contributor Author

@spouliot I'll trigger a pkg build for this.

@mandel-macaque mandel-macaque added the build-package Build (and create package) on internal Jenkins. Apply 'run-internal-tests' to run tests too. label Dec 7, 2020
@monojenkins

Copy link
Copy Markdown
Contributor

Build failure
Build failed or was aborted

Provisioning succeeded
🔥 Build failed 🔥

@monojenkins

Copy link
Copy Markdown
Contributor

Build failure
Provisioning succeeded
Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    Failed to build src/
    Search for Comparing API & creating generator diff in the log to view the complete log.
Test run succeeded

@mandel-macaque

Copy link
Copy Markdown
Contributor Author

Watch issues are known and have been fixed on #10235

Comment thread src/Foundation/NSUrlSessionHandler.cs Outdated
Co-authored-by: Rolf Bjarne Kvinge <rolf@xamarin.com>
@monojenkins

Copy link
Copy Markdown
Contributor

Build failure
Provisioning succeeded
Build succeeded
API Diff (from stable)
🔥 Failed to compare API and create generator diff 🔥
    Failed to build src/
    Search for Comparing API & creating generator diff in the log to view the complete log.
Test run succeeded

@ahhassan3

Copy link
Copy Markdown

Is there any estimate on when will this fix be released ? It's causing us many crashes

@mandel-macaque

Copy link
Copy Markdown
Contributor Author

@ahhassan3 we have not landed it because we did not had customers confirming the fix.

@mandel-macaque
mandel-macaque merged commit 6a6f1f1 into dotnet:main Jan 11, 2021
@mandel-macaque
mandel-macaque deleted the nsurlsession-handler-crash branch January 11, 2021 22:30
@ahhassan3

Copy link
Copy Markdown

@mandel-macaque This bug is somewhat difficult to reproduce on demand but we have ad over 70 counts of crashes from the customer who use our app because of this issue. I saw that you have merged it on to the main code branch, do you know when the xamarin.forms nuget will be updated with this fix in it. Thank you

mandel-macaque added a commit to mandel-macaque/xamarin-macios that referenced this pull request Mar 22, 2021
…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 <rolf@xamarin.com>
mandel-macaque added a commit that referenced this pull request Mar 22, 2021
…ixes #9132 (#10230) (#10930)

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 <rolf@xamarin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build-package Build (and create package) on internal Jenkins. Apply 'run-internal-tests' to run tests too. notes-mention Deserves a mention in release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

App crash due to NullReferenceException in NSUrlSessionHandlerDelegate.SetResponse

8 participants