-
Notifications
You must be signed in to change notification settings - Fork 625
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
deprecate NIOEventLoopGroupProvider (#2142) #2480
base: main
Are you sure you want to change the base?
Conversation
Must be merged after #2471 |
Any library that supports both Posix and Network framework will probably end up having to implement a new version of |
/// to `.share` an existing event loop group or create (and manage) a new one (`.createNew`) and let it be | ||
/// managed by given library and its lifecycle. | ||
@available(*, deprecated, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The deprecation path could be a bit painful here as it affects libraries and users. e.g. NIO can deprecate this and users are stuck with warnings until intermediate libraries update their APIs and do a release. Not sure if I'm overthinking this.
We could just deprecate the createNew
case. End users can provide .shared(MultiThreadedEventLoopGroup.singleton)
until libraries add new APIs which just take any EventLoopGroup
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kinda with George here, I think it'd be better to do a multi-stage approach. Causing all user libraries to immediately get deprecation warnings without a cycle for maintainers to add new API surface seems like it's suboptimal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just deprecate the
createNew
case. End users can provide.shared(MultiThreadedEventLoopGroup.singleton)
until libraries add new APIs which just takeany EventLoopGroup
.
Is the suggestion that libraries like Hummingbird and friends would add their own HummingBird.defaultEventLoopGroup
or something and migrate from groupProvider: EventLoopGroupProvider = .createNew
to groupProvider: EventLoopGroupProvider = .shared(HummingBird.defaultEventLoopGroup)
?
At the same time they could add a group: any EventLoopGroup? = nil
.
That would work for me. WDYT? @Lukasa / @glbrntt & @adam-fowler
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, hummingbird and co just taking an EL is not great due to the relation ship of EL to bootstrap. I think we should just create that swift-nio-universal-bootstrap
repo at some point where we expose a common way to bootstrap a client/server/datagram with a configuration that allows the user to choose .automatic
, .posix
or .niots
. With the introduction of swift-certificates
we have a way around the TLS configuration problem since we can just accept a Certificate
and then serialise and deserialise.
If we would have that hummingbird and friends would accept a UniversalServerBootstrapConfiguration
which would also include the EL configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could just deprecate the
createNew
case. End users can provide.shared(MultiThreadedEventLoopGroup.singleton)
until libraries add new APIs which just takeany EventLoopGroup
.Is the suggestion that libraries like Hummingbird and friends would add their own
HummingBird.defaultEventLoopGroup
or something and migrate fromgroupProvider: EventLoopGroupProvider = .createNew
togroupProvider: EventLoopGroupProvider = .shared(HummingBird.defaultEventLoopGroup)
?At the same time they could add a
group: any EventLoopGroup? = nil
.That would work for me. WDYT? @Lukasa / @glbrntt & @adam-fowler
Using the .shared
would be the intermediate step so that end-users can avoid warnings while libraries put out new APIs.
I think the group: any EventLoopGroup? = nil
where nil
is "choose something sensible for me" is probably the right end goal.
Why would libraries need to provide a defaultEventLoopGroup
? Seems like just the nil
case of above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would libraries need to provide a defaultEventLoopGroup? Seems like just the nil case of above.
@glbrntt Once we deprecate .createNew
, what should Hummingbird change its API too? https://github.com/hummingbird-project/hummingbird/blob/c5dd619b1cd0a2683f0dd3d4b7a0bea12485060b/Sources/Hummingbird/Application.swift#L68C1-L73C1
Hummingbird can't change to eventLoopGroupProvider: NIOEventLoopGroupProvider = .shared(MultiThreadedEventLoopGroup.singleton)
because that'll switch off NIOTS support by default. And they can't use .createNew
(because deprecated).
The only interim step that's not deprecating the whole API (which I was originally intending to do with the deprecation of NIOEventLoopGroupProvider
) is if Hummingbird provided its own .defaultEventLoopGroup
(which switches between posix & TS) and then goes for
eventLoopGroupProvider: NIOEventLoopGroupProvider = .shared(...defaultEventLoopGroup)
or am I missing smth?
Alongside that, HB probably wants to introduce a new eventLoopGroup: any EventLoopGroup? = nil
API anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would libraries need to provide a defaultEventLoopGroup? Seems like just the nil case of above.
@glbrntt Once we deprecate
.createNew
, what should Hummingbird change its API too? https://github.com/hummingbird-project/hummingbird/blob/c5dd619b1cd0a2683f0dd3d4b7a0bea12485060b/Sources/Hummingbird/Application.swift#L68C1-L73C1Hummingbird can't change to
eventLoopGroupProvider: NIOEventLoopGroupProvider = .shared(MultiThreadedEventLoopGroup.singleton)
because that'll switch off NIOTS support by default. And they can't use.createNew
(because deprecated).The only interim step that's not deprecating the whole API (which I was originally intending to do with the deprecation of
NIOEventLoopGroupProvider
) is if Hummingbird provided its own.defaultEventLoopGroup
(which switches between posix & TS) and then goes for
eventLoopGroupProvider: NIOEventLoopGroupProvider = .shared(...defaultEventLoopGroup)
or am I missing smth?
I think what you're missing is that the intermediate deprecation is to help end users, i.e. people using Hummingbird et. al., but not Hummingbird itself.
What we should try to avoid is:
- NIO is released, deprecating the provider
- User updates and has deprecation warnings but no way to avoid them
- Hummingbird et. al. get updated to provide an alternative API
- User can move to alternative API
We're fine if 2 and 3 are reversed but we can't control that, so I suggest we do the following:
- NIO is released, deprecating the
.createNew
case - User updates and has deprecation warning but can either live with the warning or use an appropriate singleton in the
.shared
case - Hummingbird et. al. get updated to provide an alternative API and deprecate their API using the provider
- User can move to new API
Alongside that, HB probably wants to introduce a new
eventLoopGroup: any EventLoopGroup? = nil
API anyway.
The new API should be this ^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we deprecate .createNew, what should Hummingbird change its API too
I guess Hummingbird would deprecate the initialiser using NIOEventLoopGroupProvider
and add a new version that took (any EventLoopGroup)?
. Although the default parameters might complicate the issue.
That's a good point.
Yes, I'll work on that rn.
I'd hope that libraries could use |
That'd work I guess |
give it a try |
Motivation:
NIOEventLoopGroupProvider.createNew
was probably never a good idea because it creates shutdown issues for any library that uses it. Given that we now have singleton (#2471)EventLoopGroup
s, we can solve this issue by just not having event loop group providers.Users can just use
group: any EventLoopGroup
and optionallygroup: any EventLoopGroup = MultiThreadedEventLoopGroup.singleton
.Modifications:
NIOEventLoopGroupProvider
Result: