NIOSingletons: Use NIO in easy mode #2471
Conversation
weissi
force-pushed
the
jw-nio-singletons
branch
from
83e12bc to
5334af4
Compare
| private let globalMultiThreadedPosixEventLoopGroup: MultiThreadedEventLoopGroup = { | ||
| let group = MultiThreadedEventLoopGroup(_canBeShutDown: false, | ||
| numberOfThreads: NIOSingletons.suggestedMultiThreadedEventLoopGroupThreadCount) | ||
| _ = Unmanaged.passUnretained(group).retain() // Never gonna give you up, |
There was a problem hiding this comment.
// Never gonna give you up,
🙉 ❤️
weissi
force-pushed
the
jw-nio-singletons
branch
5 times, most recently
from
4ae69d0 to
651d245
Compare
weissi
force-pushed
the
jw-nio-singletons
branch
from
651d245 to
a80538f
Compare
weissi
force-pushed
the
jw-nio-singletons
branch
2 times, most recently
from
0f8494c to
a148af8
Compare
adam-fowler
left a comment
adam-fowler
left a comment
There was a problem hiding this comment.
For libraries it would be great if we can replace the NIOEventLoopGroupProvider with an EventLoopGroup that defaults to one of the global ones. Then not have to worry about EventLoopGroup shutdown at all.
| /// A globally shared, lazily initialized ``NIOThreadPool`` that can be used for blocking I/O and other blocking operations. | ||
| /// | ||
| /// /// The number of threads is determined by ``NIOSingletons.suggestedBlockingPoolThreadCount``. | ||
| public static var posixBlockingPool: NIOThreadPool { |
There was a problem hiding this comment.
posixBlockingPool doesn't mean much to me. Can we name this something different, posixBlockingThreadPool maybe? I'm guessing posixThreadPool is out of the question
There was a problem hiding this comment.
@adam-fowler The default way is now NIOThreadPool.globalSingleton which I suppose is clearer?
weissi
force-pushed
the
jw-nio-singletons
branch
from
a148af8 to
592f509
Compare
weissi
force-pushed
the
jw-nio-singletons
branch
2 times, most recently
from
ff60afa to
0f5f8b9
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
Mostly docc nits but otherwise this is looking good. Still not a big fan of doubling up global and singleton though but not a hill I'll die on.
| /// This value cannot be changed using an environment variable. | ||
| /// | ||
| /// - note: This value must be set _before_ any singletons are used and must only be set once. | ||
| public static var globalSingletonsEnabledSuggestion: Bool { |
There was a problem hiding this comment.
Is this effectively a kill-switch for singletons? I.e. set this and blow up if singletons are used?
There was a problem hiding this comment.
Also, is this "suggestion" because it can be true but no singletons are made?
There was a problem hiding this comment.
@glbrntt It's a suggestion because we can't actually control that no code's creating singletons... We can make NIO code behave but we don't actually know :)
There was a problem hiding this comment.
kill switch: Yes, I think this is important to test stuff that thinks/hopes/promises to correctly wire through the group. Without the kill switch it's super duper hard to test (need to check the thread count etc)
@adam-fowler Yes, |
Just I just don't think we should do literally I'm definitely happy with
And I can probably be convinced into
And very maybe into
|
Yeah I'm okay with that. |
weissi
force-pushed
the
jw-nio-singletons
branch
from
0f5f8b9 to
1cd1d4e
Compare
glbrntt
left a comment
glbrntt
left a comment
There was a problem hiding this comment.
LGTM modulo CI failures. I think this ended up in a pretty good shape 🙂
|
@swift-nio-bot test this please |
|
ping @Lukasa, I didn't see that you actually hadn't approved this yet. Happy to make changes if there's anything. Hasn't been released yet so we've got the freedom. |
NIOSingletons: Use NIO in easy mode (apple#2471)
EventLoopGroup sIngletons were introduced in apple/swift-nio#2471
### 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 optionally `group: any EventLoopGroup = MultiThreadedEventLoopGroup.singleton`. ### Modifications: - deprecate `NIOEventLoopGroupProvider.createNew` - soft-deprecate (document as deprecated but don't mark `@available(deprecated)`) `NIOEventLoopGroupProvider` ### Result: - Libraries becomes easier to write and maintain. - Fixes #2142
8 participants
Motivation:
SwiftNIO allows and encourages to precisely manage all operating system resources like file descriptors & threads. That is a very important property of the system but in many places -- especially since Swift Concurrency arrived -- many simpler SwiftNIO programs only require a single, globally shared EventLoopGroup. Often even with just one thread.
Long story short: Many, probably most users would happily trade precise control over the threads for not having to pass around
EventLoopGroups. Today, many of those users resort to creating (and often leaking) threads because it's simpler. Adding a.globalSinglestatic var which lazily provides singletonEventLoopGroups andNIOThreadPools is IMHO a much better answer.Finally, this aligns SwiftNIO a little more with Apple's SDKs which have a lot of global singletons that hold onto system resources (
Dispatch's thread pool,URLSession.shared, ...). At least inDispatch's case the Apple SDKs actually make it impossible to manage the resources, there can only ever be one global pool of threads. That's fine for app development but wouldn't be good enough for certain server use cases, therefore I propose to addNIOSingletons as an option to the user. Confident programmers (especially in libraries) are still free and encouraged to manage all the resources deterministically and explicitly.Companion PRs:
Modifications:
NIOSingletonstypeMultiThreadedEventLoopGroup.singletonNIOThreadPool.singletonResult: