Add an option to enable Multipath TCP on clients#766
Conversation
|
Hi @Aperence, sorry for the delay in getting a review here. I'll take a look now. |
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
Thanks for this! Generally it looks like a great addition. I've noted that we need to tweak the API surface a bit.
Can I also ask that you add at least one unit test that uses the feature? That'll ensure that we don't break it.
| ignoreUncleanSSLShutdown: Bool = false, | ||
| decompression: Decompression = .disabled) { | ||
| decompression: Decompression = .disabled, | ||
| enableMultipath: Bool = false) { |
There was a problem hiding this comment.
Unfortunately, we can't add new parameters to public methods. This will need a new method that has the extra parameter.
There was a problem hiding this comment.
Does it require a new method, or do you prefer simply having the field enableMPTCP as public in the configuration, allowing to set it freely ?
| ignoreUncleanSSLShutdown: Bool = false, | ||
| decompression: Decompression = .disabled | ||
| decompression: Decompression = .disabled, | ||
| enableMultipath: Bool = false |
There was a problem hiding this comment.
Unfortunately, we can't add new parameters here. We need a new method, with the new parameter.
| ignoreUncleanSSLShutdown: Bool = false, | ||
| decompression: Decompression = .disabled) { | ||
| decompression: Decompression = .disabled, | ||
| enableMultipath: Bool = false) { |
| decompression: Decompression = .disabled, | ||
| backgroundActivityLogger: Logger?) { | ||
| backgroundActivityLogger: Logger?, | ||
| enableMultipath: Bool = false) { |
| ignoreUncleanSSLShutdown: Bool = false, | ||
| decompression: Decompression = .disabled) { | ||
| decompression: Decompression = .disabled, | ||
| enableMultipath: Bool = false) { |
Aperence
force-pushed
the
mptcp-support
branch
from
1a7f321 to
6190e9e
Compare
|
Hello @Lukasa, First, thanks for the review ! I've removed the changes to the methods' signatures. It is still possible to set the enableMultipath field as it is public, but if you prefer, I can also introduce a method for it. I've added also another test that enables MPTCP, and check that we can still fetch https://httpbin.org/get, as requested. I remain available if other changes are needed. |
|
@swift-server-bot test this please |
|
Ok, looks like the linux tests fail with |
Aperence
force-pushed
the
mptcp-support
branch
from
f26c6d2 to
b8faf9e
Compare
|
Just added the do-catch now ! |
Multipath TCP (MPTCP) is a TCP extension allowing to enhance the reliability of the network by using multiple interfaces. This extension provides a seamless handover between interfaces in case of deterioration of the connection on the original one. In the context of iOS and Mac OS X, it could be really interesting to leverage the capabilities of MPTCP as they could benefit from their multiple interfaces (ethernet + Wi-fi for Mac OS X, Wi-fi + cellular for iOS). This contribution introduces patches to HTTPClient.Configuration and establishment of the Bootstraps. A supplementary field "enableMultipath" was added to the configuration, allowing to request the use of MPTCP. This flag is then used when creating the channels to configure the client. Note that in the future, it might also be potentially interesting to offer more precise configuration options for MPTCP on MacOS, as the Network framework allows also to select a type of service, instead of just offering the option to create MPTCP connections. Currently, when enabling MPTCP, only the Handover mode is used.
Aperence
force-pushed
the
mptcp-support
branch
from
b8faf9e to
068512c
Compare
|
Applied required changes |
Co-authored-by: Cory Benfield <lukasa@apple.com>
|
@swift-server-bot test this please |
| } | ||
| let response = try client.get(url: self.defaultHTTPBinURLPrefix + "get").wait() | ||
| XCTAssertEqual(.ok, response.status) | ||
| } catch let error as IOError where error.errnoCode == EINVAL { |
There was a problem hiding this comment.
Just tried running this locally, it turns out EPROTONOSUPPORT is also sometimes thrown on Linux, so we need to add that to the list.
There was a problem hiding this comment.
Added this, I've checked the documentation and it seems that it could also raise an ENOPROTOOPT error, so I've added this one too to the list
|
@swift-server-bot test this please |
2 participants
Multipath TCP (MPTCP) is a TCP extension allowing to enhance the reliability of the network by using multiple interfaces. This extension provides a seamless handover between interfaces in case of deterioration of the connection on the original one. In the context of iOS and Mac OS X, it could be really interesting to leverage the capabilities of MPTCP as they could benefit from their multiple interfaces (ethernet + Wi-fi for Mac OS X, Wi-fi + cellular for iOS).
This contribution introduces patches to HTTPClient.Configuration and establishment of the Bootstraps. A supplementary field "enableMultipath" was added to the configuration, allowing to request the use of MPTCP. This flag is then used when creating the channels to configure the client.
Note that in the future, it might also be potentially interesting to offer more precise configuration options for MPTCP on MacOS, as the Network framework allows also to select a type of service, instead of just offering the option to create MPTCP connections. Currently, when enabling MPTCP, only the Handover mode is used.