Skip to content

[URLSession]Fix for a timeout bug(SR-2681) #911

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

Merged
merged 1 commit into from
Mar 10, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 22 additions & 4 deletions Foundation/NSURLSession/EasyHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ internal final class _EasyHandle {
fileprivate var headerList: _CurlStringList?
fileprivate var pauseState: _PauseState = []
internal var fileLength: Int64 = 0
internal var timeoutTimer: _TimeoutSource!

init(delegate: _EasyHandleDelegate) {
self.delegate = delegate
setupCallbacks()
Expand Down Expand Up @@ -387,31 +389,47 @@ fileprivate extension _EasyHandle {
}

fileprivate extension _EasyHandle {

func resetTimer() {
//simply create a new timer with the same queue, timeout and handler
//this must cancel the old handler and reset the timer
timeoutTimer = _TimeoutSource(queue: timeoutTimer.queue, milliseconds: timeoutTimer.milliseconds, handler: timeoutTimer.handler)
}

/// Forward the libcurl callbacks into Swift methods
func setupCallbacks() {
// write
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionWRITEDATA, UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque())).asError()

try! CFURLSession_easy_setopt_wc(rawHandle, CFURLSessionOptionWRITEFUNCTION) { (data: UnsafeMutablePointer<Int8>, size: Int, nmemb: Int, userdata: UnsafeMutableRawPointer?) -> Int in
guard let handle = _EasyHandle.from(callbackUserData: userdata) else { return 0 }
defer {
handle.resetTimer()
}
return handle.didReceive(data: data, size: size, nmemb: nmemb)
}.asError()
}.asError()

// read
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionREADDATA, UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque())).asError()
try! CFURLSession_easy_setopt_wc(rawHandle, CFURLSessionOptionREADFUNCTION) { (data: UnsafeMutablePointer<Int8>, size: Int, nmemb: Int, userdata: UnsafeMutableRawPointer?) -> Int in
guard let handle = _EasyHandle.from(callbackUserData: userdata) else { return 0 }
defer {
handle.resetTimer()
}
return handle.fill(writeBuffer: data, size: size, nmemb: nmemb)
}.asError()
}.asError()

// header
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionHEADERDATA, UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque())).asError()
try! CFURLSession_easy_setopt_wc(rawHandle, CFURLSessionOptionHEADERFUNCTION) { (data: UnsafeMutablePointer<Int8>, size: Int, nmemb: Int, userdata: UnsafeMutableRawPointer?) -> Int in
guard let handle = _EasyHandle.from(callbackUserData: userdata) else { return 0 }
defer {
handle.resetTimer()
}
var length = Double()
try! CFURLSession_easy_getinfo_double(handle.rawHandle, CFURLSessionInfoCONTENT_LENGTH_DOWNLOAD, &length).asError()
return handle.didReceive(headerData: data, size: size, nmemb: nmemb, fileLength: length)
}.asError()
}.asError()

// socket options
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionSOCKOPTDATA, UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque())).asError()
Expand All @@ -424,7 +442,7 @@ fileprivate extension _EasyHandle {
} catch {
return 1
}
}.asError()
}.asError()
// seeking in input stream
try! CFURLSession_easy_setopt_ptr(rawHandle, CFURLSessionOptionSEEKDATA, UnsafeMutableRawPointer(Unmanaged.passUnretained(self).toOpaque())).asError()
try! CFURLSession_easy_setopt_seek(rawHandle, CFURLSessionOptionSEEKFUNCTION, { (userdata, offset, origin) -> Int32 in
Expand Down
8 changes: 6 additions & 2 deletions Foundation/NSURLSession/MultiHandle.swift
Original file line number Diff line number Diff line change
Expand Up @@ -300,11 +300,15 @@ fileprivate extension URLSession._MultiHandle._SocketRegisterAction {

/// A helper class that wraps a libdispatch timer.
///
/// Used to implement the timeout of `URLSession.MultiHandle`.
fileprivate class _TimeoutSource {
/// Used to implement the timeout of `URLSession.MultiHandle` and `URLSession.EasyHandle`
class _TimeoutSource {
let rawSource: DispatchSource
let milliseconds: Int
let queue: DispatchQueue //needed to restart the timer for EasyHandles
let handler: DispatchWorkItem //needed to restart the timer for EasyHandles
init(queue: DispatchQueue, milliseconds: Int, handler: DispatchWorkItem) {
self.queue = queue
self.handler = handler
self.milliseconds = milliseconds
self.rawSource = DispatchSource.makeTimerSource(queue: queue) as! DispatchSource

Expand Down
16 changes: 15 additions & 1 deletion Foundation/NSURLSession/NSURLSessionTask.swift
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,14 @@ fileprivate extension URLSessionTask {
//set the request timeout
//TODO: the timeout value needs to be reset on every data transfer
let s = session as! URLSession
easyHandle.set(timeout: Int(s.configuration.timeoutIntervalForRequest))
let timeoutInterval = Int(s.configuration.timeoutIntervalForRequest) * 1000
let timeoutHandler = DispatchWorkItem { [weak self] in
guard let currentTask = self else { fatalError("Timeout on a task that doesn't exist") } //this guard must always pass
currentTask.internalState = .transferFailed
let urlError = URLError(_nsError: NSError(domain: NSURLErrorDomain, code: NSURLErrorTimedOut, userInfo: nil))
currentTask.completeTask(withError: urlError)
}
easyHandle.timeoutTimer = _TimeoutSource(queue: workQueue, milliseconds: timeoutInterval, handler: timeoutHandler)

easyHandle.set(automaticBodyDecompression: true)
easyHandle.set(requestMethod: request.httpMethod ?? "GET")
Expand Down Expand Up @@ -823,6 +830,9 @@ extension URLSessionTask {
}
self.response = response

//We don't want a timeout to be triggered after this. The timeout timer needs to be cancelled.
easyHandle.timeoutTimer = nil

//because we deregister the task with the session on internalState being set to taskCompleted
//we need to do the latter after the delegate/handler was notified/invoked
switch session.behaviour(for: self) {
Expand Down Expand Up @@ -874,6 +884,10 @@ extension URLSessionTask {
guard case .transferFailed = internalState else {
fatalError("Trying to complete the task, but its transfer isn't complete / failed.")
}

//We don't want a timeout to be triggered after this. The timeout timer needs to be cancelled.
easyHandle.timeoutTimer = nil

switch session.behaviour(for: self) {
case .taskDelegate(let delegate):
guard let s = session as? URLSession else { fatalError() }
Expand Down
4 changes: 2 additions & 2 deletions TestFoundation/HTTPServer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ class _TCPSocket {
return sockaddr_in(sin_len: 0, sin_family: sa_family_t(AF_INET), sin_port: CFSwapInt16HostToBig(port), sin_addr: in_addr(s_addr: INADDR_ANY), sin_zero: (0,0,0,0,0,0,0,0) )
#endif
}

func acceptConnection(notify: ServerSemaphore) throws {
_ = try attempt("listen", valid: isZero, listen(listenSocket, SOMAXCONN))
try socketAddress.withMemoryRebound(to: sockaddr.self, capacity: MemoryLayout<sockaddr>.size, {
Expand Down Expand Up @@ -112,7 +113,6 @@ class _TCPSocket {
for item in texts {
sleep(UInt32(sendDelay))
var bytes = Array(item.utf8)
print(item)
_ = try attempt("write", valid: isNotNegative, CInt(write(connectionSocket, &bytes, bytes.count)))
}
} else {
Expand Down Expand Up @@ -160,7 +160,7 @@ class _HTTPServer {
} else {
deadlineTime = .now()
}

DispatchQueue.main.asyncAfter(deadline: deadlineTime) {
do {
try self.socket.writeData(header: response.header, body: response.body, sendDelay: sendDelay, bodyChunks: bodyChunks)
Expand Down
4 changes: 2 additions & 2 deletions TestFoundation/TestNSURLSession.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class TestURLSession : XCTestCase {
("test_taskError", test_taskError),
("test_taskCopy", test_taskCopy),
("test_cancelTask", test_cancelTask),
// ("test_taskTimeout", test_taskTimeout),
("test_taskTimeout", test_taskTimeout),
]
}

Expand All @@ -47,7 +47,7 @@ class TestURLSession : XCTestCase {
try test.readAndRespond()
test.stop()
} catch let e as ServerError {
if e.operation != "bind" { continue }
if e.operation == "bind" { continue }
throw e
}
}
Expand Down