Implements URLSessionWebSocketTask #4643
Conversation
…ill seem worthwhile
…experimental websocket support
|
libcurl 7.86.0 was tagged earlier this week, and no API changes necessary. I think we're okay to land this. |
|
@jrflat Can I bug you for a review? |
Update "WebServices" to "WebSockets" everywhere. Update code to fail gracefully when libcurl is at >= 7.86.0, but lacks WebSockets support. Update tests to bail when WebSockets support is disabled in libcurl.
|
@swift-ci please test |
|
Looks like the Linux check was aborted, but I don't see any details why. Going to see if it'll let me kick off a test build. |
|
@swift-ci please test |
There was a problem hiding this comment.
I did a pass and I think it looks good, just some nits and questions about various imports. Were some of these changes made specifically to help swift-corelibs-foundation run on Darwin? (i.e. not necessarily part of WebSocket implementation, but useful nonetheless)
| @@ -5,7 +5,7 @@ | |||
| <key>CFBundleDevelopmentRegion</key> | |||
| <string>en</string> | |||
| <key>CFBundleExecutable</key> | |||
| <string>SwiftFoundation</string> | |||
| <string>SwiftFoundationNetworking</string> | |||
There was a problem hiding this comment.
Are these changes (SwiftFoundationXML, too) ones that should've been made on the repo earlier? (Not related to WebSocketTasks necessarily)
There was a problem hiding this comment.
Yup! As part of the e9e183c commit, these were misc changes that should have landed earlier to keep the Darwin build healthy.
| @@ -1922,6 +2007,13 @@ class TestURLSession: LoopbackServerTest { | |||
| /* ⚠️ */ testExpectedToFail(test_noDoubleCallbackWhenCancellingAndProtocolFailsFast, "This test crashes nondeterministically: https://bugs.swift.org/browse/SR-11310")), | |||
| /* ⚠️ */ ("test_cancelledTasksCannotBeResumed", testExpectedToFail(test_cancelledTasksCannotBeResumed, "Breaks on Ubuntu 18.04")), | |||
| ] | |||
| if #available(macOS 12.0, *) { | |||
There was a problem hiding this comment.
I think this was macOS 10.15 in the earlier #available() checks
There was a problem hiding this comment.
WebSocket support requires 10.15.
XCTestCase.asyncTest requires 12.0
|
|
||
| guard | ||
| let location = response.value(forHeaderField: .location), | ||
| let targetURL = URL(string: location) |
There was a problem hiding this comment.
Location can be relative. Use string:relativeTo: initializer which can handle both.
There was a problem hiding this comment.
This is existing behavior, the change here just moved redirectRequest out of an extension (see old line 656). I'd prefer to split this out into a separate issue.
|
|
||
| open func webSocketTask(with url: URL, protocols: [String]) -> URLSessionWebSocketTask { | ||
| var request = URLRequest(url: url) | ||
| request.setValue(protocols.joined(separator: ", "), forHTTPHeaderField: "Sec-WebSocket-Protocol") |
There was a problem hiding this comment.
Are Sec-WebSocket-Key and Sec-WebSocket-Version added by curl?
There was a problem hiding this comment.
Correct. -Key and -Version are properties of the WebSocket protocol itself, and are injected by libcurl. -Protocol is meant for application-specific logic on top of the WebSocket protocol, which is why this belongs at our layer.
Yes, one of the 3 commits in this PR (e9e183c) contains misc fixes to clean up compilation and running on Darwin. If preferable, I can break that out into a separate PR. But the changes were small enough that they felt better coming along for the ride as part of the larger WebSocket PR. |
Co-authored-by: Jonathan Flat <[email protected]>
|
@swift-ci please test |
Ok that makes sense I think it's fine to keep the changes in the WebSocket PR. |
| import Foundation | ||
| import XCTest | ||
|
|
There was a problem hiding this comment.
We probably do need to add these back to get the macOS build working again, but overall looks good!
There was a problem hiding this comment.
Well that's just embarrassing.
There was a problem hiding this comment.
No worries at all, tests look good!
|
@swift-ci please test |
Relies on currently-experimental web socket support in libcurl, which should land in 7.86.0. We should hold off on merging this until that version is tagged, to ensure there are no late-breaking API changes in libcurl that require changes to this patch.