Skip to content

Conversation

@mandel-macaque
Copy link
Contributor

@mandel-macaque mandel-macaque commented Feb 6, 2019

The SendAsync method in the NSUrlSessionHandler is not handling the
cancellation token correctly. Moving the Register to nearly the end of
the method (just before returning the task) ensures that if we got a
already cancelled token, we execute in a sync manner the cleanup.

Using the IsCancellationRequested in this method is less optimal
because we would have to check several times:

  1. When SencAsync receives the token and before it creates the inflight
    data.
  2. When we are going to resume the data. Since we might have created the
    inflight data and at that point an other thread cancelled the request.

Fixes: #5511

…der. Fixes dotnet#5511

The SendAsync method in the NSUtlSessionHandler is not handling the
cancellation token correctly. Moving the Register to nearly the end of
the method (just before returning the task) ensures that if we got a
already cancelled token, we execute in a sync manner the cleanup.

Using the IsCancellationRequested in this method is less optimal
because we would have to check several times:

1. When SencAsync receives the token and before it creates the inflight
data.
2. When we are going to resume the data. Since we might have created the
inflight data and at that point an other thread cancelled the request.

Fixes: dotnet#5511
@mandel-macaque
Copy link
Contributor Author

This PR provides the similar behaviour to PR #5334 as well as it ensures that if the token is cancelled at some other point in time while executing SendAsync it is respected.

I believe this is a nicer fix.

@rolfbjarne rolfbjarne changed the title [Foundation] Remove possible race condition with the NSUrlSessionHanlder. Fixes #5511 [Foundation] Remove possible race condition with the NSUrlSessionHandler. Fixes #5511 Feb 6, 2019
Copy link
Member

@dalexsoto dalexsoto left a comment

Choose a reason for hiding this comment

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

❤️ it! nice one.

@monojenkins
Copy link
Collaborator

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

@mandel-macaque mandel-macaque merged commit b87be15 into dotnet:master Feb 6, 2019
@mandel-macaque mandel-macaque deleted the nsurlsession-cancelation branch February 6, 2019 16:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants