Skip to content

Add data(from:delegate:) method. #4705

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

Closed
wants to merge 1 commit into from
Closed

Add data(from:delegate:) method. #4705

Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions Sources/FoundationNetworking/URLSession/FTP/FTPURLProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ internal extension _FTPURLProtocol {
self.client?.urlProtocol(self, didReceive: response, cacheStoragePolicy: .notAllowed)
case .dataCompletionHandler:
break
case .dataCompletionHandlerWithTaskDelegate:
break
case .downloadCompletionHandler:
break
}
Expand Down
2 changes: 2 additions & 0 deletions Sources/FoundationNetworking/URLSession/HTTP/HTTPURLProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -525,6 +525,8 @@ internal class _HTTPURLProtocol: _NativeProtocol {
}
case .dataCompletionHandler:
break
case .dataCompletionHandlerWithTaskDelegate:
break
case .downloadCompletionHandler:
break
}
Expand Down
3 changes: 2 additions & 1 deletion Sources/FoundationNetworking/URLSession/NativeProtocol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,8 @@ internal class _NativeProtocol: URLProtocol, _EasyHandleDelegate {
// Data will be forwarded to the delegate as we receive it, we don't
// need to do anything about it.
return .ignore
case .dataCompletionHandler:
case .dataCompletionHandler,
.dataCompletionHandlerWithTaskDelegate:
// Data needs to be concatenated in-memory such that we can pass it
// to the completion handler upon completion.
return .inMemory(nil)
Expand Down
2 changes: 2 additions & 0 deletions Sources/FoundationNetworking/URLSession/TaskRegistry.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@ extension URLSession {
case callDelegate
/// Default action for all events, except for completion.
case dataCompletionHandler(DataTaskCompletion)
/// Default action for all asynchronous events.
case dataCompletionHandlerWithTaskDelegate(DataTaskCompletion, URLSessionTaskDelegate?)
/// Default action for all events, except for completion.
case downloadCompletionHandler(DownloadTaskCompletion)
}
Expand Down
36 changes: 36 additions & 0 deletions Sources/FoundationNetworking/URLSession/URLSession.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -442,6 +442,34 @@ open class URLSession : NSObject {
open func dataTask(with url: URL, completionHandler: @escaping (Data?, URLResponse?, Error?) -> Void) -> URLSessionDataTask {
return dataTask(with: _Request(url), behaviour: .dataCompletionHandler(completionHandler))
}

@available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *)
open func data(from url: URL, delegate: URLSessionTaskDelegate? = nil) async throws -> (Data, URLResponse) {
var task: URLSessionTask? = nil

return try await withTaskCancellationHandler {
try await withCheckedThrowingContinuation { continuation in
let completionHandler: URLSession._TaskRegistry.DataTaskCompletion = { data, response, error in
if let data, let response {
continuation.resume(returning: (data, response))
} else if let error {
continuation.resume(throwing: error)
} else {
fatalError()
}
}
task = dataTask(with: _Request(url), behaviour: .dataCompletionHandlerWithTaskDelegate(completionHandler, delegate))

if Task.isCancelled {
continuation.resume(throwing: CancellationError())
return
}
task?.resume()
}
} onCancel: { [weak task] in
task?.cancel()
}
}

/* Creates an upload task with the given request. The body of the request will be created from the file referenced by fileURL */
open func uploadTask(with request: URLRequest, fromFile fileURL: URL) -> URLSessionUploadTask {
Expand Down Expand Up @@ -648,6 +676,9 @@ internal extension URLSession {
/// Default action for all events, except for completion.
/// - SeeAlso: URLSession.TaskRegistry.Behaviour.dataCompletionHandler
case dataCompletionHandler(URLSession._TaskRegistry.DataTaskCompletion)
/// Default action for all asynchronous events.
/// - SeeAlso: URLsession.TaskRegistry.Behaviour.dataCompletionHandlerWithTaskDelegate
case dataCompletionHandlerWithTaskDelegate(URLSession._TaskRegistry.DataTaskCompletion, URLSessionTaskDelegate)
/// Default action for all events, except for completion.
/// - SeeAlso: URLSession.TaskRegistry.Behaviour.downloadCompletionHandler
case downloadCompletionHandler(URLSession._TaskRegistry.DownloadTaskCompletion)
Expand All @@ -656,6 +687,11 @@ internal extension URLSession {
func behaviour(for task: URLSessionTask) -> _TaskBehaviour {
switch taskRegistry.behaviour(for: task) {
case .dataCompletionHandler(let c): return .dataCompletionHandler(c)
case .dataCompletionHandlerWithTaskDelegate(let c, let d):
guard let d else {
return .dataCompletionHandler(c)
}
return .dataCompletionHandlerWithTaskDelegate(c, d)
case .downloadCompletionHandler(let c): return .downloadCompletionHandler(c)
case .callDelegate:
guard let d = delegate as? URLSessionTaskDelegate else {
Expand Down
10 changes: 6 additions & 4 deletions Sources/FoundationNetworking/URLSession/URLSessionTask.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ private class Bag<Element> {

/// A cancelable object that refers to the lifetime
/// of processing a given request.
open class URLSessionTask : NSObject, NSCopying {
open class URLSessionTask : NSObject, NSCopying, @unchecked Sendable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this class been audited for thread-safety to get this Sendable annotation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Foundation on Apple platforms, URLSessionTask has already conformed to Sendable. So, I naturally assume that the URLSessionTask in Core Foundation could also conform to Sendable. Do you think any detailed audits are necessary?

If we need additional audits, I'd like to contribute to it. However, I'm not sure how to audit for thread-safety of this class. Could you please give me your advice?

Copy link
Contributor

Choose a reason for hiding this comment

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

In Foundation on Apple platforms, URLSessionTask has already conformed to Sendable. So, I naturally assume that the URLSessionTask in Core Foundation could also conform to Sendable.

URLSessionTask is implemented in Foundation proper, it's not present in CoreFoundation. You can confirm that by searching the CoreFoundation directory.

Do you think any detailed audits are necessary?

Yes, of course. If something is marked as Sendable in Foundation on Darwin platforms, it means that an audit was done and necessary changes that make it thread-safe applied if needed on it. Since these are different codebases, it has to be done separately in this repository too.

If we need additional audits, I'd like to contribute to it. However, I'm not sure how to audit for thread-safety of this class. Could you please give me your advice?

I don't think there's anything specific to this repository when compared to a general thread-safety audit on an arbitrary codebase. As a bare minimum one would have to confirm that mutable state accessible from different threads is either synchronized or made immutable. If there's a class hierarchy involved, this has to be done to mutable state inherited from superclasses too. In Swift I'd probably start with making a type Sendable (not @unchecked Sendable) and then auditing and adding matching sendability annotations to types involved in those warnings.


// These properties aren't heeded in swift-corelibs-foundation, but we may heed them in the future. They exist for source compatibility.
open var countOfBytesClientExpectsToReceive: Int64 = NSURLSessionTransferSizeUnknown {
Expand Down Expand Up @@ -1058,7 +1058,7 @@ extension _ProtocolClient : URLProtocolClient {
webSocketDelegate.urlSession(session, webSocketTask: webSocketTask, didOpenWithProtocol: webSocketTask.protocolPicked)
}
}
case .noDelegate, .dataCompletionHandler, .downloadCompletionHandler:
case .noDelegate, .dataCompletionHandler, .dataCompletionHandlerWithTaskDelegate, .downloadCompletionHandler:
break
}
}
Expand Down Expand Up @@ -1157,7 +1157,8 @@ extension _ProtocolClient : URLProtocolClient {
session.workQueue.async {
session.taskRegistry.remove(task)
}
case .dataCompletionHandler(let completion):
case .dataCompletionHandler(let completion),
.dataCompletionHandlerWithTaskDelegate(let completion, _):
session.delegateQueue.addOperation {
guard task.state != .completed else { return }
completion(urlProtocol.properties[URLProtocol._PropertyKey.responseData] as? Data ?? Data(), task.response, nil)
Expand Down Expand Up @@ -1297,7 +1298,8 @@ extension _ProtocolClient : URLProtocolClient {
session.workQueue.async {
session.taskRegistry.remove(task)
}
case .dataCompletionHandler(let completion):
case .dataCompletionHandler(let completion),
.dataCompletionHandlerWithTaskDelegate(let completion, _):
session.delegateQueue.addOperation {
guard task.state != .completed else { return }
completion(nil, nil, error)
Expand Down
11 changes: 11 additions & 0 deletions Tests/Foundation/Tests/TestURLSession.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,16 @@ class TestURLSession: LoopbackServerTest {
task.resume()
waitForExpectations(timeout: 12)
}

func test_dataFromURLDelegate() async throws {
guard #available(macOS 12.0, iOS 15.0, watchOS 8.0, tvOS 15.0, *) else { return }
let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/UK"
let (data, response) = try await URLSession.shared.data(from: URL(string: urlString)!, delegate: nil)
guard let httpResponse = response as? HTTPURLResponse else { return }
XCTAssertEqual(200, httpResponse.statusCode, "HTTP response code is not 200")
let expectedResult = String(data: data, encoding: .utf8) ?? ""
XCTAssertEqual("London", expectedResult, "Did not receive expected value")
}

func test_dataTaskWithHttpInputStream() throws {
let urlString = "http://127.0.0.1:\(TestURLSession.serverPort)/jsonBody"
Expand Down Expand Up @@ -2100,6 +2110,7 @@ class TestURLSession: LoopbackServerTest {
]
if #available(macOS 12.0, *) {
retVal.append(contentsOf: [
("test_dataFromURLDelegate", asyncTest(test_dataFromURLDelegate)),
("test_webSocket", asyncTest(test_webSocket)),
("test_webSocketSpecificProtocol", asyncTest(test_webSocketSpecificProtocol)),
("test_webSocketAbruptClose", asyncTest(test_webSocketAbruptClose)),
Expand Down