Fix for concurrent fetching bugs #1227
Conversation
designatednerd
changed the title
| let taskData = TaskData(rawCompletion: rawTaskCompletionHandler, | ||
| completionBlock: completion) | ||
|
|
||
| self.tasks.mutate { $0[task.taskIdentifier] = taskData } |
There was a problem hiding this comment.
I would create some "insert" that checks if ID is already there just for sanity sake :)
or have something like
self.tasks.mutate {
// assert if id is in $0
$0[task.taskIdentifier] = taskData
}There was a problem hiding this comment.
generally i'd agree with this but I did get confirmation that the issue was my misuse of locks that was causing the problem with the task data failing to increment. If this isn't working now, it's a clear URLSession bug.
There was a problem hiding this comment.
(I also was able to validate that with the changes the number increments properly)
There was a problem hiding this comment.
the problem might arise if someone changes locking logic and will reintroduce bug. Without any checking error will be just "swallowed" until someone reports it :)
There was a problem hiding this comment.
Fair, though I think that's probably better to check through tests than an assertion
There was a problem hiding this comment.
updated the test with concurrentPerform to validate that all task ids created are unique.
| self.datas.value[dataTask.taskIdentifier]?.append(data) | ||
| self.tasks.mutate { | ||
| guard let taskData = $0[dataTask.taskIdentifier] else { | ||
| return |
There was a problem hiding this comment.
would personally add some assert as it "must" be here
There was a problem hiding this comment.
I've been really trying to avoid asserts in library code here, but I do think it's reasonable for the didReceive data method.
There was a problem hiding this comment.
assert is not great for reason that they don't play nicely with tests... but just ignoring possible error is not nice...
There was a problem hiding this comment.
yeah, this is where not being able to throw an error is really annoying. I did add the assert to the didReceive data method.
| @@ -0,0 +1,25 @@ | |||
| import Foundation | |||
|
|
|||
| public class TaskData { | |||
There was a problem hiding this comment.
wonder if you need do some "cleanup" on deinit (like calling completion handlers)
There was a problem hiding this comment.
There's no way to tell if the completion handlers have already been called, so I don't think that's a great idea in this case.
…session during a test
…o have made that actually work properly
designatednerd
force-pushed
the
fix/multifetch
branch
from
3445ac4 to
e58853d
Compare
|
OK, I'm gonna merge this - @RolandasRazma thanks for the review! |
any time :) |
|
@prakash8393 Can you please open a new issue on this so we can figure out what's going on? It'd also help to see code from the place you're calling it, so we can see if maybe something's getting hit by ARC. |
Thanks @designatednerd it worked . thanks for your response :) |
So, turns out a fairly large assumption I made (URLSessionTaskidentifers are unique) is basically incorrect in a sufficiently concurrent set of requests. This means when using the task identifier as a key in a dictionary, you may wind up with multiple requests having the same task identifier, and the dictionaries lose their mind.This PR updates the underlying dictionaries inURLSessionClientto key off theURLSessionTaskitself rather than just the identifier.This is a partial fix for #1210 - there's still something going on there that I can't put my finger on, but I wanted to at least get this merged for the next version since it does fix what is definitely incorrect behaviorWelp, looks like our locking behavior wasn't doing what I was expecting it to do, and it was having all sorts of weird side effects, including effing up the task identifier generation - shout out to @davedelong and @bdash for spotting what I was doing wrong and offering better suggestions.
This should now be an actual fix for #1210 and/or #1226.