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

Lower EventLoopGroup requirements for creating bootstraps #49

Merged
merged 2 commits into from
Jul 3, 2019

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Jun 21, 2019

Motivation:

In NIO ClientBootstrap and ServerBootstrap are initialized with an
EventLoopGroup. Since EventLoop conforms to EventLoopGroup you can
initialize an bootstrap with an existing event loop. In NIO Transport
Services the bootstraps are initialized with a NIOTSEventLoopGroup and
as NIOTSEventLoop conforms to EventLoop and by extension
EventLoopGroup (but not NIOTSEventLoopGroup) it is not possible to
initialize a bootstrap with a pre-existing NIOTSEventLoop.

Modifications:

Change the bootstrap initializers to accept an EventLoopGroup.

Result:

NIO Transport Services bootstraps can be initialized with a
NIOTSEventLoop.

@glbrntt
Copy link
Contributor Author

glbrntt commented Jul 3, 2019

cc @weissi

@weissi weissi added the semver/minor Adds new public API. label Jul 3, 2019
@weissi
Copy link
Member

weissi commented Jul 3, 2019

looks SemVer minor to me because this compiles:

protocol EventLoopGroup {}

struct NIOTSEventLoop: EventLoopGroup {}

func foo(_ c: EventLoopGroup) {}

let f: (NIOTSEventLoop) -> Void = foo

but @glbrntt to verify using NIO's check_no_api_breakages.sh

Copy link
Member

@weissi weissi left a comment

Choose a reason for hiding this comment

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

looks good to me! I don't really like the PR because it makes it easier to pass a wrong event loop that won't work but what you intend to do is entirely reasonable so this must work. Also it brings it more in line with NIO core. Real fix is SemVer major and needs to be designed alongside apple/swift-nio#674

Motivation:

In NIO `ClientBootstrap` and `ServerBootstrap` are initialized with an
`EventLoopGroup`. Since `EventLoop` conforms to `EventLoopGroup` you can
initialize an bootstrap with an existing event loop. In NIO Transport
Services the bootstraps are initialized with a `NIOTSEventLoopGroup` and
as `NIOTSEventLoop` conforms to `EventLoop` and by extension
`EventLoopGroup` (but not `NIOTSEventLoopGroup`) it is not possible to
initialize a bootstrap with a pre-existing `NIOTSEventLoop`.

Modifications:

Change the bootstrap initializers to accept an `EventLoopGroup`.

Result:

NIO Transport Services bootstraps can be initialized with a
`NIOTSEventLoop`.
@glbrntt
Copy link
Contributor Author

glbrntt commented Jul 3, 2019

Not a fan either but I don't know of a better way to do this!

I also added comments to the initialisers taking EventLoopGroup to explain their existence.

@weissi
Copy link
Member

weissi commented Jul 3, 2019

Thanks @glbrntt , having some explanation is great!

@glbrntt
Copy link
Contributor Author

glbrntt commented Jul 3, 2019

Also I tested API breakage on a local branch which removed commit afbbead because of #50 -- I can check again once #51 is merged if you like?

@weissi weissi merged commit 23e77df into apple:master Jul 3, 2019
@glbrntt glbrntt deleted the event-loop-group branch July 4, 2019 11:06
@Lukasa Lukasa added this to the 1.1.0 milestone Jul 22, 2019
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.

3 participants