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

Async methods for NIORawSocketBootstrap #2460

Merged
merged 3 commits into from
Jul 5, 2023

Conversation

FranzBusch
Copy link
Member

Motivation

We want to support async bootstrapping with all our bootstraps.

Modification

This PR adds support for the NIORawSocketBootstrap and adds the three variants of methods to it (abstract output, NIOAsyncChannel based and protocol negotiation based)

Result

We now support asynchronous interaction with the NIORawSocketBootstrap

@FranzBusch FranzBusch requested review from glbrntt, dnadoba and Lukasa July 4, 2023 10:26
/// - Returns: A `NIOAsyncChannel` for the bound connection.
@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
@_spi(AsyncChannel)
public func bind<Inbound: Sendable, Outbound: Sendable>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we be providing the initialiser for this (and connect) here?

We have a split world where you can specify it in the initialiser on the bootstrap and in some cases (protocol negotiation and the "return whatever you want from the initializer") in bind/connect.

It might be easier if all the async bind/connect methods have initialisers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah realised this today as well but we gotta revisit it holistically. I will defer this to the overall API review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we not add them with inits in the other NIO bootstraps though?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, the NIOAsyncChannel based ones are all without an explicit initialiser

Copy link
Contributor

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

LGTM modulo one nit

/// - Returns: A `NIOAsyncChannel` for the bound connection.
@available(macOS 10.15, iOS 13, tvOS 13, watchOS 6, *)
@_spi(AsyncChannel)
public func bind<Inbound: Sendable, Outbound: Sendable>(
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we not add them with inits in the other NIO bootstraps though?

@FranzBusch FranzBusch force-pushed the fb-raw-socket-bootstrap branch from b8786cc to 2a94c6e Compare July 4, 2023 12:54
@FranzBusch FranzBusch added the semver/none No version bump required. label Jul 4, 2023
@FranzBusch FranzBusch force-pushed the fb-raw-socket-bootstrap branch 2 times, most recently from b63704c to 2bebb36 Compare July 4, 2023 14:15
# Motivation
We want to support async bootstrapping with all our bootstraps.

# Modification
This PR adds support for the `NIORawSocketBootstrap` and adds the three variants of methods to it (abstract output, `NIOAsyncChannel` based and protocol negotiation based)

# Result
We now support asynchronous interaction with the `NIORawSocketBootstrap`
@FranzBusch FranzBusch force-pushed the fb-raw-socket-bootstrap branch from 2bebb36 to f8dbe9e Compare July 4, 2023 14:18
@FranzBusch FranzBusch enabled auto-merge (squash) July 5, 2023 12:37
@FranzBusch FranzBusch merged commit 555194e into apple:main Jul 5, 2023
@FranzBusch FranzBusch deleted the fb-raw-socket-bootstrap branch July 5, 2023 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/none No version bump required.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants