Skip to content

[URLSession](SR-4567) Fix for a race condition in URLSession #949

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Apr 19, 2017
Merged

[URLSession](SR-4567) Fix for a race condition in URLSession #949

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 37 additions & 19 deletions Foundation/NSURLSession/MultiHandle.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,11 @@ extension URLSession {
let group = DispatchGroup()
fileprivate var easyHandles: [_EasyHandle] = []
fileprivate var timeoutSource: _TimeoutSource? = nil


//SR-4567: we need to synchronize the register/unregister commands to the epoll machinery in libdispatch
fileprivate let commandQueue: DispatchQueue = DispatchQueue(label: "Register-unregister synchronization")
fileprivate var cancelInProgress: DispatchSemaphore? = nil

init(configuration: URLSession._Configuration, workQueue: DispatchQueue) {
queue = DispatchQueue(label: "MultiHandle.isolation", target: workQueue)
setupCallbacks()
Expand Down Expand Up @@ -99,25 +103,33 @@ fileprivate extension URLSession._MultiHandle {
// through libdispatch (DispatchSource) and store the source(s) inside
// a `SocketSources` which we in turn store inside libcurl's multi handle
// by means of curl_multi_assign() -- we retain the object fist.
let action = _SocketRegisterAction(rawValue: CFURLSessionPoll(value: what))
var socketSources = _SocketSources.from(socketSourcePtr: socketSourcePtr)
if socketSources == nil && action.needsSource {
let s = _SocketSources()
let p = Unmanaged.passRetained(s).toOpaque()
CFURLSessionMultiHandleAssign(rawHandle, socket, UnsafeMutableRawPointer(p))
socketSources = s
} else if socketSources != nil && action == .unregister {
// We need to release the stored pointer:
if let opaque = socketSourcePtr {
Unmanaged<_SocketSources>.fromOpaque(opaque).release()
commandQueue.async {
self.cancelInProgress?.wait()
Copy link
Contributor

Choose a reason for hiding this comment

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

how long could cancels take? this could back up this queue indefinitely.

Copy link
Member Author

@pushkarnk pushkarnk Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks @phausler for the review

In my experience, I haven't seen DispatchSource.cancel() blocking for a long time. Moreover, the intention here is to block the command queue until a cancel() goes through completely, of course assuming that cancel() will not block. I have considered the fact that the cancel handler runs on the same queue as the event handler. The event handler will do two things:

  1. read/write from/to sockets that are ready - this will not block
  2. issue further register/unregister commands - this is async deferred to the command queue
    So, in my understanding the event handler will not block the cancel handler - which means the wait() here will not block indefinitely.

@dgrove-oss Can DispatchSource.cancel() can block under any other circumstances?

Copy link
Contributor

Choose a reason for hiding this comment

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

If the only work that falls out is just DispatchSource.cancel() this is a non issue; it effectively is an atomic assign of a value.

self.cancelInProgress = nil
Copy link
Contributor

Choose a reason for hiding this comment

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

the assignment here is perhaps not thread safe since the cancel handler gets a reference to it captured in the tearDown method; is it possible to instead have the semaphore last as long as the containing object context that is retained past the lifespan of the execution on the queue?

Copy link
Member Author

@pushkarnk pushkarnk Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The nil assignment is guarded by the wait() and only the cancel handler will signal() - which means the nil assignment is executed only when the cancel handler no longer needs the semaphore. The assignment to nil signifies that there is no cancel operation in progress which allows the register commands go through without waiting, until the next unregister. Increasing the scope of the semaphore will lead to other register commands (may be on unrelated sockets) to wait with perhaps nobody to signal them.

Copy link
Contributor

Choose a reason for hiding this comment

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

there are three parts to the state of that semaphore; nil, not nil, and the signal. It doesn't look like a huge race potential but there is still something that strikes me as a potential failure here ( hard to tell without more understanding of exactly the involved queues/threads) This isn't a blocking objection - just a word of caution (and perhaps a suggestion to add a comment).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I'll add a comment around this code.


let action = _SocketRegisterAction(rawValue: CFURLSessionPoll(value: what))
var socketSources = _SocketSources.from(socketSourcePtr: socketSourcePtr)
if socketSources == nil && action.needsSource {
let s = _SocketSources()
let p = Unmanaged.passRetained(s).toOpaque()
CFURLSessionMultiHandleAssign(self.rawHandle, socket, UnsafeMutableRawPointer(p))
socketSources = s
} else if socketSources != nil && action == .unregister {
//the beginning of an unregister operation
self.cancelInProgress = DispatchSemaphore(value: 0)
// We need to release the stored pointer:
if let opaque = socketSourcePtr {
Unmanaged<_SocketSources>.fromOpaque(opaque).release()
}
socketSources?.tearDown(self.cancelInProgress)
socketSources = nil
}
socketSources = nil
}
if let ss = socketSources {
let handler = DispatchWorkItem { [weak self] in
self?.performAction(for: socket)
if let ss = socketSources {
let handler = DispatchWorkItem { [weak self] in
self?.performAction(for: socket)
}
ss.createSources(with: action, fileDescriptor: Int(socket), queue: self.queue, handler: handler)
}
ss.createSources(with: action, fileDescriptor: Int(socket), queue: queue, handler: handler)
}
return 0
}
Expand Down Expand Up @@ -398,12 +410,18 @@ fileprivate class _SocketSources {
s.resume()
}

func tearDown() {
func tearDown(_ cancelInProgress: DispatchSemaphore?) {
let cancelHandler = DispatchWorkItem {
//the real end of an unregister operation!
cancelInProgress?.signal()
}
if let s = readSource {
s.setCancelHandler(handler: cancelHandler)
s.cancel()
}
readSource = nil
if let s = writeSource {
s.setCancelHandler(handler: cancelHandler)
s.cancel()
}
writeSource = nil
Expand Down