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
make sure writes to URLSession.taskRegistry happen on work queue #1625
Conversation
@swift-ci please test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. I remember seeing some extremely intermittent crashes around task registry. They may well be due to the absence of this synchronisation.
@swift-ci please test and merge |
Thanks @ahti ! |
@ahti @pushkarnk can we cherry pick this to 4.2 branch? |
I would say yes, but I'm not yet familiar enough with the codebase to say so with much authority ^_^ |
Please put up a PR and we'll get it merged. Otherwise, the fix won't be released until Swift 5 next year. |
Foundation: backport apple/swift-corelibs-foundation#2061
session.taskRegistry.remove(task) | ||
} | ||
} | ||
case .noDelegate: | ||
task.state = .completed | ||
session.taskRegistry.remove(task) | ||
session.workQueue.async { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for necroposting but this and this https://github.com/apple/swift-corelibs-foundation/pull/1625/files#diff-14c6cb724eead661596e4468ae5ca638R625 looks like overkill
There is no thread switching with session.delegateQueue.addOperation
And if this method itself not synchronized race will be earlier at line 545
Foundation: backport apple/swift-corelibs-foundation#1625
Addresses SR-8017.
The docs for taskRegistry specify any use should occur on the sessions workQueue, which wasn't respected with some of the removals.
Some of the removals previously happened on the tasks work queue, which has the session work queue as target and thus wouldn't cause issues, but I decided to make those explicitly use the sessions queue for clarity as well.
I'm not sure all callers of
URLSession.behaviour(for:)
, the other place the taskRegistry is accessed, are run on the work queue, but the changes in this PR resolved the crashes I was seeing, so I didn't touchURLSession.behaviour(for:)
.A test for this would be nice, but I couldn't really think of a way to test this, as the crashes occur randomly and require maybe a few thousand requests to semi-reliably reproduce.