/AFNetworking

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

When tasks are created on a concurrent queue, incorrect completionHandlers can get called. #2093

Closed
PadraigK opened this Issue May 25, 2014 · 7 comments

Comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@PadraigK @mattt @nikitahils @kcharwood
@PadraigK
Copy link
Contributor

PadraigK commented May 25, 2014

Due to this bug: http://openradar.appspot.com/radar?id=5871104061079552 in NSURLSessionTask, creating tasks on a concurrent queue can cause incorrect completionHandlers to get called.

When a duplicate taskIdentifier is returned by the task, the previous completionHandler gets cleared out and replaced with the new one. If the data for the first request comes back before the second request's data, the first response is then called against the second completionHandler.

I'm not sure what AFNetworking should do here — it could enforce creating tasks on a serial queue or it could just advise people to do so. We could also add a test to assert that the taskIdentifier is not a duplicate?
@mattt

This comment has been minimized.

Sign in to view
Copy link
Contributor

mattt commented Jun 11, 2014

Would it make sense for AFNetworking to manage its own internal serial queue, and dispatch all task creation calls into that?
@PadraigK

This comment has been minimized.

Sign in to view
Copy link
Contributor

PadraigK commented Jun 11, 2014

Yeah, that would certainly work. I'll update this task if my radar gets closed.

On 11 Jun 2014, at 13:33, Mattt Thompson notifications@github.com wrote:

Would it make sense for AFNetworking to manage its own internal serial queue, and dispatch all task creation calls into that?

—
Reply to this email directly or view it on GitHub.

mattt added a commit that referenced this issue Jun 22, 2014

@mattt
[Issue #2093] Using serial queue to synchronize creation of session t… 
…asks to prevent bug where NSURLSession creates tasks with non-unique identifiers (see radr://5871104061079552)
Loading status checks…
a563ca0
@mattt

This comment has been minimized.

Sign in to view
Copy link
Contributor

mattt commented Jun 22, 2014

Implemented the aforementioned serial queue solution in a563ca0. Let me know if you see any problems or have any suggestions on how to improve it. Thanks!

@mattt mattt closed this Jun 22, 2014

@PadraigK

This comment has been minimized.

Sign in to view
Copy link
Contributor

PadraigK commented Jul 31, 2014

Just FYI, this bug is fixed in iOS 8.
@nikitahils

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikitahils commented Dec 7, 2015

@matt, we know that bug fixed in iOS 8+, should we support creation_queue in iOS 8+ versions?
May be we should separate this logic depending on iOS version?
For example:

  1. if iOS == 7.x: create synchronously task on serial creation_queue (additional logic for fixing this bug)
  2. if iOS >= 8.x: create task (no need in additional logic, here is this bug fixed)
@kcharwood

This comment has been minimized.

Sign in to view
Copy link
Contributor

kcharwood commented Dec 7, 2015

We could potentially go down that route. If you wanted to work up a PR on the 3.0.0 branch, I'd be happy to review it!
@nikitahils

This comment has been minimized.

Sign in to view
Copy link
Contributor

nikitahils commented Dec 8, 2015

@kcharwood, it's a good idea, i'll try to deliver this fix on this weekend!

nikitahils added a commit to nikitahils/AFNetworking that referenced this issue Dec 8, 2015

@nikitahils
Non-blocking task creation feature on iOS 8 + versions with support o… 
…f iOS 7 version with additional logic of fixing Apple bug in NSURLSession.

More about:
Open Radar: http://openradar.appspot.com/radar?id=5871104061079552 (status: Fixed in iOS8)
Issue: AFNetworking#2093
1f9c0f9

@nikitahils nikitahils referenced this issue Dec 8, 2015

Closed

Task creation feature (logic improved for non-buggy versions of NSURLSession) #3205

@kcharwood kcharwood referenced this issue Dec 10, 2015

Merged

Improved task creation performance for iOS 8+ #3208

@arttrick arttrick referenced this issue Aug 10, 2017

Merged

Fix AFURLSessionManager security weakness #3962

Ner1Co added a commit to Ner1Co/AFNetworking that referenced this issue Sep 6, 2017

@Ner1Co
AFURLSessionManager: Move the workaround attempts inside safe block. 
Static Analyzer raised a warning that
`uploadTaskWithRequest:fromFile:progress:completionHandler:` can return
`nil` although its return type is not nullable.

The method `url_session_manager_create_task_safely` is receiving a block
that runs the uploading task under the conditions:
if iOS == 7.x: create synchronously task on serial creation_queue.
if iOS >= 8.x: just create the task.
in order to overcome another iOS 7 bug:
AFNetworking#2093

However the workaround attempts that should overcome another bug:
AFNetworking#1675
are not inside the safely method.
4a49ba0

Ner1Co added a commit to Ner1Co/AFNetworking that referenced this issue Sep 11, 2017

@Ner1Co
Static analyzer raises a warning that 
`uploadTaskWithRequest:fromFile:progress:completionHandler:` can return
`nil` although its return type is declared to be nonnull.
Inside `uploadTaskWithRequest:fromFile:progress:completionHandler:` the
creation of the upload task is done using the method
`url_session_manager_create_task_safely` passing it a block that creates
the task. This function is used to workaround a bug in iOS 7.x
(AFNetworking#2093), where task creation must be performed on a single
thread.

However there's another bug in iOS < 8.0 that causes
`NSURLSession` to sometimes return `nil` when creating a file upload
task (AFNetworking#1675). This bug has another workaround that retry to
create the task few more times. However these retry attempts are not
executed via the safety function mentioned above, potentially
reinstating the other iOS 7 bug.

This commit moves the task creation retry attempts to inside the block
that is sent to `url_session_manager_create_task_safely` and thus
ensuring retry attempts if needed, will also run on the designated
queue.
62b042a

Ner1Co added a commit to Ner1Co/AFNetworking that referenced this issue Sep 12, 2017

@Ner1Co
AFURLSessionManager: Fix static analyzer issues. 
Static analyzer raises a warning that
`uploadTaskWithRequest:fromFile:progress:completionHandler:` can return
`nil` although its return type is declared to be nonnull.
Inside `uploadTaskWithRequest:fromFile:progress:completionHandler:` the
creation of the upload task is done using the method
`url_session_manager_create_task_safely` passing it a block that creates
the task. This function is used to workaround a bug in iOS 7.x
(AFNetworking#2093), where task creation must be performed on a single
thread.

However there's another bug in iOS < 8.0 that causes
`NSURLSession` to sometimes return `nil` when creating a file upload
task (AFNetworking#1675). This bug has another workaround that retry to
create the task few more times. However these retry attempts are not
executed via the safety function mentioned above, potentially
reinstating the other iOS 7 bug.

This commit moves the task creation retry attempts to inside the block
that is sent to `url_session_manager_create_task_safely` and thus
ensuring retry attempts if needed, will also run on the designated
queue.
d11ac96
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment