[Foundation] Ensure that if a request is canceled we cancel the NSUrlSessionTask

[mandel-macaque]

This change fixes bug #53794

If the request is canceled, the inflight data is cleaned up. That allows
us to know that the NSUrlSessionTask should be canceled, which will
execute the DidCompleteWithError that will take care of the cleanup of
resources and will ensure that everything is back to a known situation.

Bug description: - https://bugzilla.xamarin.com/show_bug.cgi?id=53794

[spouliot]

inflight? not needed, it cannot be null anymore

[mandel-macaque]

True, removing the ?.

[spouliot]

should that check be moved to GetInflightData so we check the token every time it's called ? IOW is this needed outside DidReceiveResponse ?

[mandel-macaque]

Well, I moved the task.Cancel there, to GetInflightData, at that point, the DidCompleteWithError handler will be executed, everything will be cleaned. AS per docs, the DidReceiveResponse gets called just when we got the headers (or at least that is what I understand from https://developer.apple.com/reference/foundation/nsurlsessiondatadelegate/1410027-urlsession?language=objc) .

What can happen is that we got the headers AND suddenly (if even possible) then got canceled, so we check there. Next time we will deal with the data from DidReceiveData (https://developer.apple.com/reference/foundation/nsurlsessiondatadelegate/1411528-urlsession?language=objc) in that case, we will get data BUT we also have to check if the inflight is null to ensure that we did not get canceled in the middle of the data being received, if we did get canceled, again, DidCompleteWithError will be taking care of everything, else, we wait for another DidReceiveData, and again, we could have been canceled again.

So, the check for the inflight data to be null has to be done in each of the Handlers that are receiving information from the task, check it, ensure that we got the data, if not, return.

I moved the task cancelation to the GetInflightData, since it makes no sense to do that check, cancel every time, but I don't see a "nice" way to avoid the check in the handlers since we could be canceled in the middle of each of the events.

So, what I'm thinking is that in a multithreaded situation we could have:

Handler is called -> Get Canceled -> Continue with handler

So ideally we should do

Handler is called -> Get Canceled -> Check inflight data -> Continue with handler

and getting the inflight data already will cancel the NSUrlsTask if the inflight data was cleaned. That data can be cleaned by several reasons:

1. Got an error form the os, and the handler is cleaning stuff.
2. CancellationToken was used (line 177)
3. The content was disposed during the DidReciveResponse handler (line 239)
4. An exception occurred in the DidReceiveResponse (line 277)

[spouliot]

I agree the inflight null check is needed everywhere GetInflightData is called.

I also agree that GetInflightData is also the best place to handle cancellations.

My point is that it only deals with one case, i.e. there are two sources of cancellation, the ones we get from the nativeNSUrlSession* itself, and the ones from the managed CancellationToken .

The later is only handled in DidReceiveResponse, but if that checks is moved into GetInflightData then it would be checked every time to OS call us back, which seems better. E.g. should not DidReceiveData also check CancellationToken ?

[mandel-macaque]

Ok, I agree that we should move the check of the cancelation token in the inflight data to the GetInflightData method, makes sense, since if we do have the cancelation, we can cancel de NSUrlSessionTask.

This posses an interesting dilema, we can have data from the DidReceiveResponse or DidReceiveData in the inflight data EVEN when the cancelation was requested. In that case, what should we do? We have two options:

1. Get the inflight data, check the cancelation token, cancel if needed and return the data for the handler to deal with. This means that we could be adding headers or body data in the canceled request, but is data we already have and I see no real reason to not do it.
2. Get the inflight data, cancel the task AND return null, instead of the data. In this case, we might be loosing the headers and some body data because the inflight data is cleaned and the task canceled.

I'm more inclined to the first option. For example, we got the headers, and canceled, the user will get the headers and will get the issue in the async read of the stream of the response.

In summary we are choosing between:

     InflightData GetInflightData (NSUrlSessionTask task)
     {
 	var inflight = default (InflightData);
 
 	lock (sessionHandler.inflightRequestsLock)
  		if (sessionHandler.inflightRequests.TryGetValue (task, out inflight)) {
                        if (inflight.CancellationToken.IsCancellationRequested)
			    task?.Cancel ();
  			return inflight;
                }
        task?.Cancel ();
  	return null;
    }

And the second one:

     InflightData GetInflightData (NSUrlSessionTask task)
     {
 	var inflight = default (InflightData);
 
 	lock (sessionHandler.inflightRequestsLock)
  		if (sessionHandler.inflightRequests.TryGetValue (task, out inflight)) {
                        if (inflight.CancellationToken.IsCancellationRequested) {
			    task?.Cancel ();
                            inflight = null;
                        }
                        return inflight;
                }
        task?.Cancel ();
  	return null;
    }

[spouliot]

@mandel-macaque Let's go with the first once, it's closer to the existing behaviour.

[monojenkins]

Build failure

[monojenkins]

Build failure

[monojenkins]

Build success

[monojenkins]

Build failure

[spouliot]

known random failure -> https://bugzilla.xamarin.com/show_bug.cgi?id=52173

[dotMorten]

Thank you !