Skip to content
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

Add configuration of multipathServiceType in NIOTSConnectionBootstrap #205

Merged
merged 2 commits into from
Sep 18, 2024

Conversation

Aperence
Copy link
Contributor

Motivation:

Allow different devices to leverage the capabilities of Mutipath TCP (MPTCP) to enhance the network reliability. Thanks to MPTCP, a connection can for example automatically migrate to another interface if it deteriorates on the first one. This can be especially interesting on MacOS X and iOS, where devices may frequently benefit from multiple interfaces (ethernet + Wi-Fi for Macs and Wi-fi + cellular for iOS). Allowing developers to enable MPTCP on their connections seems thus like a fine addition to this library.

Modifications:

Added a function "withMultipath" on NIOTSConnectionBootstrap, that allow to configure the type of service used for MPTCP (defaults to disabled). This value will be stored in a field, and then propagated to the underlying channel when the connect method will be called. Also updated the parameters field of NIOTSConnectionChannel to set the multipathServiceType accordingly.

Result:

Users will now be able to easily enable Multipath TCP, allowing them to benefit from seamless handover, interactive mode to automatically use the lowest delay interface or aggregate mode to send data in parallel on both interfaces.

Example:

let group = NIOTSEventLoopGroup()
defer {
     try! group.syncShutdownGracefully()
}
let bootstrap = NIOTSConnectionBootstrap(group: group)
    .withMultipath(.handover)   // set handover mode
    .channelInitializer { channel in
        channel.pipeline.addHandler(MyChannelHandler())
    }
try! bootstrap.connect(host: "example.org", port: 12345).wait()
/* the Channel is now connected */

@Lukasa Lukasa added the semver/minor Adds new public API. label Aug 23, 2024
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks for filing this! This is a nice addition, as offering a convenience API for multipath service type is useful. However, the multipath service type is already configurable via the NIOTSChannelOptions.multipathServiceType option. I'm all for offering a convenience abstraction for this, but I recommend we have it decay down to just setting the channel option, as that will ensure we only have a single path for getting this to work correctly.

@Aperence
Copy link
Contributor Author

Thank you for your feedback, I'll try to simplify it to use the channel options !

@Aperence
Copy link
Contributor Author

Aperence commented Aug 23, 2024

Hello,

I've simplified (a lot) the code thanks to your feedback, it now uses the NIOTSChannelOptions, limiting drastically the modifications needed

@Aperence Aperence requested a review from Lukasa August 24, 2024 14:41
Copy link
Collaborator

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Thanks for this, and sorry for the delay in getting to it! Can you briefly add a unit test that configures both a listener and a connection bootstrap and connects them, and checks the set values using the channel options? I'd just like to make sure we don't accidentally regress this API in future.

@Aperence
Copy link
Contributor Author

No problem, I've just added this test now !

…Bootstrap

Motivation:

Allow different devices to leverage the capabilities of Mutipath TCP (MPTCP) to
enhance the network reliability. Thanks to MPTCP, a connection can for example
automatically migrate to another interface if it deteriorates on the first one.
This can be especially interesting on MacOS X and iOS, where devices may frequently
benefit from multiple interfaces (ethernet + Wi-Fi for Macs and Wi-fi + cellular for
iOS). Allowing developers to enable MPTCP on their connections seems thus like a fine
addition to this library.

Modifications:

Added a function "withMultipath" on NIOTSConnectionBootstrap and
NIOTSListenerBootstrap, that allow to configure the type of service used for MPTCP
(defaults to disabled). This will then set the appropriate channel option, which will
be propagated to the underlying channel.

Result:

Users will now be able to easily enable Multipath TCP, allowing them to benefit from
seamless handover, interactive mode to automatically use the lowest delay interface or
aggregate mode to send data in parallel on both interfaces.
@Lukasa
Copy link
Collaborator

Lukasa commented Sep 18, 2024

@swift-server-bot test this please

@Lukasa Lukasa enabled auto-merge (squash) September 18, 2024 16:48
@Lukasa Lukasa merged commit afd1691 into apple:main Sep 18, 2024
5 of 6 checks passed
@Lukasa
Copy link
Collaborator

Lukasa commented Sep 18, 2024

Thank you very much @Aperence, and thanks for being very game with the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants