Update NSUrlSessionHandler to cancel request if provided cancellationToken is already canceled. #5334

jonathanantoine · 2018-12-28T14:20:23Z

Update NSUrlSessionHandler to cancel request if provided cancellationToken is already canceled.

An already cancelled request is no more sent and the task nicely canceled.

It fix this issue #5333 on my tests.

…Token is already canceled Update NSUrlSessionHandler to cancel request if provided cancellationToken is already canceled. An already cancelled request is no more sent and the task nicely canceled. It fix this issue xamarin#5333 on my tests.

dnfclas · 2018-12-28T14:20:35Z

All CLA requirements met.

chamons · 2018-12-28T15:35:22Z

Thanks for the contribution!

As this is not my area of expertise, we'll have to wait for others with more experience in networking to land this. Given that many are on Christmas break, it may be a week or so until everyone is back.

Apologies for the delay and thanks again.

spouliot · 2019-01-02T20:24:16Z

build

src/Foundation/NSUrlSessionHandler.cs

spouliot

Please provide more explanation on how you think this fix the issue and how it can be verified. Thanks!

spouliot · 2019-01-02T21:11:48Z

src/Foundation/NSUrlSessionHandler.cs

-				dataTask.Resume ();
-
+
+			if (!cancellationToken.IsCancellationRequested) {


That seems to always be hit (e.g. adding an else and a breakpoint or some Console.WriteLine is never hit).

Also I can reproduce the hang locally, with the supplied test case, even with the patch applied. Now it works, some times, but it's hard to say if the different timings makes it more or less likely (or if it's purely random).

Considering the Cancel is a no-op I do not think this change is fixing the issue.

What is always hit? I use the reproduce steps provided in the referred bug and with this it does not happens. Be sure to use a small timeout btw

if (!cancellationToken.IsCancellationRequested) {

is always evaluated as true when testing, so in effect that change is a no op

I used different times and the results varies (hangs or none) but it's to really correlated to the times.

Ok I get it : do you want me to remove this line or can we consider that in theory the cancellationToken could be already cancelled as you don't know from where it comes from ?

I think the line can stay, it's logic not to add it if already cancelled.

However this makes both of your original changes no ops which means nothing is solved. If you're getting different results it's because the test case is a bit too random in its results (not your fault but it makes things harder to see if the problem is fixed).

If this is related to the mono thread pool issue (like you cross-linked to) then it's unlikely that ay change in the handler code will be able to fix the issue.

You are right, I have made more investigation and it seems that this is another behavior which causes the issue : #5334 (comment)

monojenkins · 2019-01-02T21:30:51Z

Build failure
✅ Build succeeded
✅ API Diff (from stable)
ℹ️ API Diff (from PR only) (please review changes)
✅ Generator Diff (no change)
🔥 Test run failed 🔥

Test results

1 tests failed, 0 tests skipped, 80 tests passed.

Failed tests

monotouch-test/iOS Unified 64-bits - simulator/Release (all optimizations): Failed

Put the CreateDataTask call inside the inflightRequestsLock to be sure that NSUrlSessionHandlerDelegate::DidCompleteWithError will get it if the task is created immediatly "faulted" by iOS.

jonathanantoine · 2019-01-03T12:40:12Z

@spouliot I have made more tests on my side wit the provided sample for the issue. I have found out that ios can also create a "faulted task" when calling session.CreateDataTask.

In this case, the inflightRequests does not contains the created task when the NSUrlSessionHandlerDelegate is called (it's 'DidCompleteWithError' method).

To fix this, I added the creation of the task in the inflightRequestsLock and then the data will be for sure in it.

When not's done, it seems that in SendAsync the await tcs.Task.ConfigureAwait(false); line never ends.

How can I help you more ?

spouliot · 2019-01-04T13:48:39Z

src/Foundation/NSUrlSessionHandler.cs

-				if (dataTask.State == NSUrlSessionTaskState.Suspended)
+				}
+
+				if (dataTask != null && dataTask.State == NSUrlSessionTaskState.Suspended)


why the addition of dataTask != null ? have you run into that case ?

it happens if cancellationToken.IsCancellationRequested which could theorically happen.

no it can't since you moved that block (if) inside the condition (you mentioned)

OMG you are right sorry for being this blind reading my own code ! I removed the null check.

Is it ok for you @spouliot ? How can I help you on this PR ? :)

remove useless null check

mandel-macaque · 2019-01-08T11:58:36Z

@jonathanantoine @spouliot let me poke the test application and will try to get back to you asap (sorry for the delay, xmas break).

mandel-macaque · 2019-01-08T14:33:21Z

@spouliot @jonathanantoine so, I don't how do you feel about this approach, but it is something that I have tested (I quickly wrote a patch that you can find here) and seems to work, or at least, do not hang the application.

Let me first explain the train of tough:

We have a user application that has NOT created a background NSUrlSession and does try to perform a number of requests.
The user sets the application in the background and then returns.
The application is in a non recovery state (and I think this is an issue with the thread pool from mono as you pointed out).

The ideal fix would be that we do not have an issue with the thread pool, but this is not going to be something we can fix, at least in the sort time. The new ideal fix, or at least, what I though it would be a good compromise, would be to cancel those request that are inflight. We could do so by listening to the notification center, register a callback that will try to set the inflight sources to be canceled, that will trigger the registration of the cancellation token and should clean the resources. At this point, our API user will know that the task was cancelled and that he will have to deal with that, if a developer does not want the task to be canceled when the app is in the background, simply point to the background session.

Regarding the PR, I'm +1 on not adding a task if the cancelation token tells us.

mandel-macaque

+1 on adding the if statement, but I don't believe that it does fix the issue. Please read my comment for a possible workaround (cancelling when being notified of the app going to the background).

jonathanantoine · 2019-01-08T15:45:28Z

@mandel-macaque thanks for your review. I don't understand the change you want as the "if" statement is present and used.

mandel-macaque · 2019-01-08T17:27:43Z

@jonathanantoine take a look at the gist I provided. while the check for the cancelation is indeed needed, the addition is not fixing the underlaying issue. As I mentioned on
#5334 (comment) we can write a solution that will never hang. The idea, if you do not have a background session, tasks are cancelled and cleaned up. It will be up to the app writer to deal with the cancellation,

This way you will always be in a known state and the state machine will not hang/block.

chamons · 2019-02-06T13:59:59Z

Hey @jonathanantoine it turns out the issue you ran into runs a bit deeper than first suspected. #5511 for example.

@mandel-macaque started with your work and extended it to cover that case as well, which is now #5557.

Sorry for the long delay in responding to this PR, we wanted to make sure we had this nailed down before we closed your PR. Thanks for the contribution and being a part of fixing this fun network issue.

jonathanantoine · 2019-02-06T18:21:00Z

Perfect, thanks a lot for your work !

jonathanantoine requested review from chamons, rolfbjarne and spouliot as code owners December 28, 2018 14:20

chamons requested a review from mandel-macaque December 28, 2018 15:33

chamons assigned chamons and unassigned chamons Dec 28, 2018

minor code style update

c1c9401

spouliot reviewed Jan 2, 2019

View reviewed changes

src/Foundation/NSUrlSessionHandler.cs Outdated Show resolved Hide resolved

spouliot suggested changes Jan 2, 2019

View reviewed changes

Update NSUrlSessionHandler.cs

f66207c

Put the CreateDataTask call inside the inflightRequestsLock to be sure that NSUrlSessionHandlerDelegate::DidCompleteWithError will get it if the task is created immediatly "faulted" by iOS.

Remove useless call to dataTask?.Cancel ();

86ff4bc

spouliot reviewed Jan 4, 2019

View reviewed changes

Update NSUrlSessionHandler.cs

5da4cce

remove useless null check

mandel-macaque requested changes Jan 8, 2019

View reviewed changes

mandel-macaque mentioned this pull request Jan 23, 2019

[Foundation] Ensure that we do not block when in the background without a background session. #5463

Merged

mandel-macaque mentioned this pull request Feb 6, 2019

[Foundation] Remove possible race condition with the NSUrlSessionHandler. Fixes #5511 #5557

Merged

chamons closed this Feb 6, 2019

jonathanantoine deleted the master-fix-5333 branch February 6, 2019 18:21

dalexsoto added the community Community contribution ❤ label Feb 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update NSUrlSessionHandler to cancel request if provided cancellationToken is already canceled. #5334

Update NSUrlSessionHandler to cancel request if provided cancellationToken is already canceled. #5334

jonathanantoine commented Dec 28, 2018

dnfclas commented Dec 28, 2018 •

edited

Loading

chamons commented Dec 28, 2018

spouliot commented Jan 2, 2019

spouliot left a comment

spouliot Jan 2, 2019

jonathanantoine Jan 2, 2019

spouliot Jan 3, 2019

jonathanantoine Jan 3, 2019

spouliot Jan 3, 2019

jonathanantoine Jan 3, 2019

monojenkins commented Jan 2, 2019

Failed tests

jonathanantoine commented Jan 3, 2019

spouliot Jan 4, 2019

jonathanantoine Jan 4, 2019

spouliot Jan 4, 2019

jonathanantoine Jan 4, 2019 •

edited

Loading

jonathanantoine Jan 5, 2019

mandel-macaque commented Jan 8, 2019

mandel-macaque commented Jan 8, 2019 •

edited

Loading

mandel-macaque left a comment

jonathanantoine commented Jan 8, 2019

mandel-macaque commented Jan 8, 2019

chamons commented Feb 6, 2019

jonathanantoine commented Feb 6, 2019

		dataTask.Resume ();


		if (!cancellationToken.IsCancellationRequested) {

Update NSUrlSessionHandler to cancel request if provided cancellationToken is already canceled. #5334

Update NSUrlSessionHandler to cancel request if provided cancellationToken is already canceled. #5334

Conversation

jonathanantoine commented Dec 28, 2018

dnfclas commented Dec 28, 2018 • edited Loading

chamons commented Dec 28, 2018

spouliot commented Jan 2, 2019

spouliot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

monojenkins commented Jan 2, 2019

Test results

Failed tests

jonathanantoine commented Jan 3, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanantoine Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mandel-macaque commented Jan 8, 2019

mandel-macaque commented Jan 8, 2019 • edited Loading

mandel-macaque left a comment

Choose a reason for hiding this comment

jonathanantoine commented Jan 8, 2019

mandel-macaque commented Jan 8, 2019

chamons commented Feb 6, 2019

jonathanantoine commented Feb 6, 2019

dnfclas commented Dec 28, 2018 •

edited

Loading

jonathanantoine Jan 4, 2019 •

edited

Loading

mandel-macaque commented Jan 8, 2019 •

edited

Loading