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

Jump to bottom

Create a ClientBootstrap protocol #1253

Merged

Lukasa merged 2 commits into apple:master from Yasumoto:yasumoto-client-bootstrap-protocol

Dec 11, 2019

Contributor

Yasumoto commented Nov 19, 2019

Create a simple protocol for Client-side bootstrap implementations to conform to.

Motivation:

As described in #674, we want to make it easier to switch between ClientBootstrap and NIOTSConnectionBootstrap.

This is one approach that I'm intrigued by, but not necessarily overly fond of. We're still allowing folks to use the wrong type of EventLoopGroup with the bootstrap they choose. However, this simplifies the ability to use both types of bootstrap objects, and at the least serves as a strawman proposal.

Modifications:

Created a ClientTransportBootstrap protocol. I think it makes sense to separate out a client vs. server since the usecases and surface area are so different.

Result:

Users will be able to choose between either ClientBootstrap or NIOTSConnectionBootstrap much more easily.

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

8 similar comments

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

swift-server-bot commented Nov 19, 2019

Can one of the admins verify this patch?

This was referenced Nov 19, 2019

Conform to ClientTransportBootstrap apple/swift-nio-transport-services#65

Closed

Support NIO Transport Services swift-server/async-http-client#135

Closed

Lukasa requested a review from weissi

November 19, 2019 17:55

Lukasa requested changes

View reviewed changes

Contributor

Lukasa left a comment

This mostly looks really good, thanks! In general I think this is a good idea, but we'd need to add some unit testing for this as well to confirm that it is possible to actually use this protocol correctly.

Sources/NIO/Bootstrap.swift Outdated

+                  ///
+                  /// - parameters:
+                  ///     - group: The `EventLoopGroup` to use.
+                  init(group: EventLoopGroup)

Contributor

Lukasa Nov 19, 2019

In an ideal world we'd have a proper type here, but I am really struggling to come up with a way to do it. I'm going to think on it some more and maybe an idea will come to me.

Contributor Author

Yasumoto Nov 20, 2019

Agreed, please take your time 🙇‍♂️

I was wondering if we could use an associated type and force something, but I don't think that's correct. I'm hoping to figure out how to save folks from accidentally using a MultiThreadedEventLoopGroup with `NIOTSConnectionBootstrap. 🤔

Member

weissi Nov 20, 2019

I think we should remove the init here and instead add a public static func makeTCPClientBootstrap() -> NIOClientTransportBootstrap to EventLoopGroup.

Sure, for API compatibility we need to implement a default implementation for this which would be

extension EventLoopGroup {
    public static func makeTCPClientBootstrap() -> NIOClientTransportBootstrap {
        struct CannotBootstrap: NIOClientTransportBootstrap {
            // fail everything with a good error
        }
        return CannotBootstrap()
    }
}

and in NIO 3 we can remove that. WDYT @Lukasa / @Yasumoto ?

Member

weissi Nov 20, 2019

The reason for removing the init is because there's less room for error. The new way would basically be to choose your favourite EventLoopGroup and from there you could peel off a TCP client bootstrap.

Member

weissi Nov 20, 2019

We would then just implement this for SelectableEventLoop, MultiThreadedEventLoopGroup, EmbeddedEventLoop, and NIOTSEventLoop(Group) which is 99.999% of the actual use-cases so nobody will ever see the error.

Contributor

Lukasa Nov 20, 2019

At a certain point though I wonder if we're over-solving this. For now it may be sufficient to exclude everything but TCP clients and servers and to solve those cases today. We can try to generalise further later.

Member

weissi Nov 20, 2019

@Lukasa agreed. We could just fly with basically what's there and name it accordingly leaving the more general name available for the next version?

Member

weissi Nov 20, 2019

I would however remove the init and make users peel off the bootstrap from the ELG maybe?

Contributor

Lukasa Nov 20, 2019

I'm fine with users peeling off from the ELG for now. Let's go down that road and worry about the best possible ergonomics later.

Member

weissi Nov 20, 2019

sounds good to me! @Yasumoto are you okay with this and does this make sense?

Sources/NIO/Bootstrap.swift Outdated

@@ @@ -325,6 +325,47 @@ private extension Channel { @@
                   }
               }
+              /// `ClientTransportBootstrap` is implemented by various underlying transport mechanisms. Typically,
+              /// this will be the BSD Sockets API implemented by `ClientBootstrap`.
+              public protocol ClientTransportBootstrap {

Contributor

Lukasa Nov 19, 2019

This would need to be called NIOClientTransportBootstrap to meet our API naming guidelines.

Yasumoto commented

View reviewed changes

Tests/NIOTests/BootstrapTest.swift Outdated

@@ @@ -205,4 +205,38 @@ class BootstrapTest: XCTestCase { @@
                       XCTAssertNoThrow(try childChannelDone.futureResult.wait())
                       XCTAssertNoThrow(try serverChannelDone.futureResult.wait())
                   }
+                  func testClientTransportBootstrapAllowsConformanceCorrectly() throws {

Contributor Author

Yasumoto Nov 20, 2019

I was inspired by ChannelTests.testBasicLifecycle, happy to add more/different test cases if anyone has other ideas.

Lukasa requested changes

View reviewed changes

Tests/NIOTests/BootstrapTest.swift Outdated

+                      defer {
+                          XCTAssertNoThrow(try group.syncShutdownGracefully())
+                      }
+                      let clientBootstrap: NIOClientTransportBootstrap = ClientBootstrap(group: group)

Contributor

Lukasa Nov 20, 2019

It might be nicer to write a function that takes the bootstrap generically and write the test in that function. That way it's much harder to accidentally break this test by changing only one line in a seemingly-innocuous way.

weissi requested changes

View reviewed changes

Member

weissi left a comment

I think this is a really good idea to start with a simple TCP client bootstrapping protocol. We can always improve.

Sources/NIO/Bootstrap.swift Outdated

+                  ///
+                  /// - parameters:
+                  ///     - group: The `EventLoopGroup` to use.
+                  init(group: EventLoopGroup)

Member

weissi Nov 20, 2019

The reason for removing the init is because there's less room for error. The new way would basically be to choose your favourite EventLoopGroup and from there you could peel off a TCP client bootstrap.

Sources/NIO/Bootstrap.swift Outdated

@@ @@ -325,6 +325,47 @@ private extension Channel { @@
                   }
               }
+              /// `NIOClientTransportBootstrap` is implemented by various underlying transport mechanisms. Typically,
+              /// this will be the BSD Sockets API implemented by `ClientBootstrap`.
+              public protocol NIOClientTransportBootstrap {

Member

weissi Nov 20, 2019

naming nit: maybe NIOTCPClientBootstrap? Because this bootstrap will only work for TCP stuff. But not strong on this one, @Lukasa ?

Contributor

Lukasa Nov 20, 2019

I might prefer NIOStreamClientBootstrap, to enable the possibility of encapsulating other things.

Sources/NIO/Bootstrap.swift Outdated

+                  ///
+                  /// - parameters:
+                  ///     - group: The `EventLoopGroup` to use.
+                  init(group: EventLoopGroup)

Member

weissi Nov 20, 2019

We would then just implement this for SelectableEventLoop, MultiThreadedEventLoopGroup, EmbeddedEventLoop, and NIOTSEventLoop(Group) which is 99.999% of the actual use-cases so nobody will ever see the error.

Contributor Author

Yasumoto commented Nov 21, 2019

Naming

name it accordingly leaving the more general name available for the next version?

I assumed this meant go with NIOTCPStreamClientBootstrap vs. NIOStreamClientBootstrap (or something else) so it was very specific, if that's not right lemme know.

Default Implementation

I'm not sure the proposal for a default CannotBootstrap is correct, since none of the methods throw. Here's what I came up with:

extension EventLoopGroup {
    public static func makeTCPClientBootstrap() -> NIOTCPStreamBootstrap {
        struct CannotBootstrap: NIOTCPStreamBootstrap {
            func channelInitializer(_ handler: @escaping (Channel) -> EventLoopFuture<Void>) -> CannotBootstrap {
                return self
            }

            func channelOption<Option>(_ option: Option, value: Option.Value) -> CannotBootstrap where Option : ChannelOption {
                return self
            }

            func connectTimeout(_ timeout: TimeAmount) -> CannotBootstrap {
                return self
            }

            func connect(host: String, port: Int) -> EventLoopFuture<Channel> {
               /* return ???? */
            }
        }
        return CannotBootstrap()
    }
}

Static

I also thought we do not want this to be static, as we want to provide the actual ELG to the bootstrap.

Where to implement `makeTCPClientBootstrap`

I assumed we only want this on MultiThreadedEventLoopGroup and NIOTSEventLoop, but should we also add it to SelectableEventLoop, EmbeddedEventLoop, and NIOTSEventLoop?

Merge commit

🤦‍♂ I'll squash after we get the questions sorted a bit more, I rebased instead of merged.

Member

weissi commented Nov 21, 2019 •

edited

Loading

Thanks for looking into this!!

Naming

name it accordingly leaving the more general name available for the next version?

I assumed this meant go with NIOTCPStreamClientBootstrap vs. NIOStreamClientBootstrap (or something else) so it was very specific, if that's not right lemme know.

Right now, it only supports TCP so I think it should be NIOTCPClientBootstrap or something like that. @Lukasa ?

Default Implementation

I'm not sure the proposal for a default CannotBootstrap is correct, since none of the methods throw. Here's what I came up with:

extension EventLoopGroup {
    public static func makeTCPClientBootstrap() -> NIOTCPStreamBootstrap {
        struct CannotBootstrap: NIOTCPStreamBootstrap {
            func channelInitializer(_ handler: @escaping (Channel) -> EventLoopFuture<Void>) -> CannotBootstrap {
                return self

I'd return a failed future here

        }

        func channelOption<Option>(_ option: Option, value: Option.Value) -> CannotBootstrap where Option : ChannelOption {
            return self
        }

        func connectTimeout(_ timeout: TimeAmount) -> CannotBootstrap {
            return self
        }

        func connect(host: String, port: Int) -> EventLoopFuture<Channel> {
           /* return ???? */

return self.next().makeFailedFuture(CannotBootstrapError(#"Your EventLoopGroup of type \(type(of: self)) is not compatible with "NIOTCPStreamBootstrap", please implement "public func makeTCPClientBootstrap() -> NIOTCPStreamBootstrap" "#))

should do it.

        }
    }
    return CannotBootstrap()
}

}


### Static
I also thought we do not want this to be `static`, as we want to provide the actual ELG to the bootstrap.

Agreed, shouldn't be static.

Where to implement makeTCPClientBootstrap

I assumed we only want this on MultiThreadedEventLoopGroup and NIOTSEventLoop, but should we also add it to SelectableEventLoop, EmbeddedEventLoop, and NIOTSEventLoop?

First, declare it in the EventLoopGroup protocol, then, add a default implementation using extension EventLoopGroup to retain API compatibility.

Merge commit

🤦‍♂ I'll squash after we get the questions sorted a bit more, I rebased instead of merged.

We can also do that in the github UI at the very end.

Contributor

Lukasa commented Nov 21, 2019

NIOTCPClientBootstrap sounds fine.

Contributor

kylebrowning commented Dec 9, 2019

How is this going? Im desperately looking forward to this for https://github.com/apple/swift-nio-transport-services

Yasumoto force-pushed the yasumoto-client-bootstrap-protocol branch 2 times, most recently from 5744ee6 to 79a4479 Compare

December 10, 2019 05:51

Contributor Author

Yasumoto commented Dec 10, 2019

How is this going? Im desperately looking forward to this for https://github.com/apple/swift-nio-transport-services

Thanks for the nudge, @kylebrowning ! 😅

Went through the feedback again, and I believe I've incorporated it, so this should be ready for a fresh look.

Member

weissi commented Dec 10, 2019

@Yasumoto I'm happy taking this if we make it internal for the time being, sorry about that.

The reason we can't use this as-is is because it doesn't support TLS at all. That would mean that we have a very easy to use and compatible version for plain text (which nobody should ever use) and if you need TLS, you need to go back to the old, #if canImport(Network)... way. I have some ideas how we could solve this but it's too much for this PR. So if you'd be happy with that, we could start with what we have here but all internal, then I add what's necessary for TLS and then we make it all public in one go. CC @Lukasa does that sound like a plan?

Contributor

Lukasa commented Dec 10, 2019

Works for me!

Yasumoto added 2 commits

December 10, 2019 09:58


          Create a protocol for the ClientBootstrap.

36776c2

This allows other EventLoopGroup implementations to swap in other
bootstrap implementations. By default, it is unimplemented and will
throw an error once `connect` is called on the returned Bootstrap.

Support is implemented for `MultiThreadedEventLoopGroup`,
`SelectableEventLoop` and `EmbeddedEventLoop`.


          Make this internal

69f2dd4

Yasumoto force-pushed the yasumoto-client-bootstrap-protocol branch from 79a4479 to 69f2dd4 Compare

December 10, 2019 18:53

Contributor Author

Yasumoto commented Dec 10, 2019 •

edited

Loading

Awesome, totally a great save. Thanks for helping get this over the finish line!

weissi approved these changes

View reviewed changes

Member

weissi left a comment

thanks so much @Yasumoto !

weissi requested a review from Lukasa

December 11, 2019 08:50

Lukasa approved these changes

View reviewed changes

Contributor

Lukasa left a comment

Let's ship this.

Lukasa added the semver/patch label

Lukasa added this to the 2.12.0 milestone

Contributor

Lukasa commented Dec 11, 2019

@swift-nio-bot test this please

Lukasa merged commit 06dab4e into apple:master

Yasumoto deleted the yasumoto-client-bootstrap-protocol branch

January 14, 2020 05:21

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels