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

narrower bootstrap ELG requirements #674

Closed
tanner0101 opened this issue Nov 30, 2018 · 7 comments
Closed

narrower bootstrap ELG requirements #674

tanner0101 opened this issue Nov 30, 2018 · 7 comments
Labels
⚠️ semver/major Breaks existing public API.
Milestone

Comments

@tanner0101
Copy link
Contributor

tanner0101 commented Nov 30, 2018

NIO's bootstrap helpers like ClientBootstrap and ServerBootstrap currently accept any generic EventLoopGroup. However, the event loops created by this group are eventually force casted to a SelectableEventLoop. This effectively limits one to using MultiThreadedEventLoop or EmbeddedEventLoop.

I suggest narrowing the event loop groups accepted by these bootstraps to something like a SelectableEventLoopGroup protocol. That would help move some obvious programming errors, like passing a NIOTS* event loop group to a standard NIO bootstrap, from runtime to compile time.

@Lukasa
Copy link
Contributor

Lukasa commented Dec 1, 2018

While we can do this, I don’t know that it provides much utility. The issue is that any protocol we make here will be implementable by others, and so will not be a particularly good type constraint.

I’m a bit more inclined to say that we should consider whether we can do something more sensible here: for example, given that in practice our Bootstraps are tightly tied to specific loop and channel implementations, perhaps they should be created by those loops and channels, rather than accepting them in the constructor.

@weissi
Copy link
Member

weissi commented Dec 1, 2018

We should definitely think this through and make it more sensible. The quick fix (as Tanner describes) would be vastly better than what we have today because you don’t get weird error messages. As Thomas on the Vapor channel was pointing out, our docs for ClientBootstrap currently say it’s the best way to create tcp connections which is only partly true.
However I agree with Cory that we should look into doing something more holistic. Even though the risk of someone creating a random event loop group type isn’t nearly as likely to hit as someone passing the wrong NIO provided ELG type.

So, if nothing else we should do the protocol but I’m sure we can come up with some proper solution which doesn’t allow for any ‘wrong type’ crashes there. Kind of like getting the bootstrap from the ELG, that might also allow us to have a more unified bootstrap protocol that works for NIO and NIOTS

@weissi weissi added this to the 2.0.0 milestone Dec 3, 2018
@weissi weissi added the ⚠️ semver/major Breaks existing public API. label Feb 25, 2019
@weissi weissi removed this from the 2.0.0 milestone Feb 25, 2019
@weissi weissi added this to the 3.0.0 milestone Apr 12, 2019
@Lukasa
Copy link
Contributor

Lukasa commented May 3, 2019

I think the best option here will be if the opaque result types proposal lands in Swift: in that case, we'll be able to use associated types on the EventLoopGroup protocol to make it clearer which groups can work with which bootstraps. Alternatively, we'll be able to define a bootstrap protocol that can constrain the groups: either way would be better than today.

@Yasumoto
Copy link
Contributor

Wanted to chime in (and hopefully help future searchers!) that I ran into this today trying to use NIOTSEventLoopGroup with AsyncHTTPClient, which bailed out at a force cast.

Could not cast value of type 'NIOTransportServices.NIOTSEventLoop' (0x10051b098) to 'NIO.SelectableEventLoop' (0x100515088).
2019-07-16 18:56:29.216181-0700 LoadGenerator[6098:13214129] Could not cast value of type 'NIOTransportServices.NIOTSEventLoop' (0x10051b098) to 'NIO.SelectableEventLoop' (0x100515088).

Totally possible there's a way to do this I'm not realizing, in which case I'm happy to update some docs/pointers somewhere!

@Lukasa
Copy link
Contributor

Lukasa commented Jul 17, 2019

Currently there is not. AsyncHTTPClient hard-codes the ClientBootstrap, which is inherently a construct that requires a MultiThreadedEventLoopGroup. Going forward we really do need to be able to define a protocol for bootstraps to allow for them to be provided to the code that sets up the connections, but the same discussion above applies.

@weissi
Copy link
Member

weissi commented Jun 10, 2020

We have got ClientBootstrap(validating:) and ServerBootstrap(validating:) so users can use that instead.

If that's not good enough, please file a new bug.

@weissi weissi closed this as completed Jun 10, 2020
@Yasumoto
Copy link
Contributor

So awesome, thanks for all the hard work, folks!! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠️ semver/major Breaks existing public API.
Projects
None yet
Development

No branches or pull requests

4 participants