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

NIOSingletonsTransportServices: Use NIOTS in easy mode #180

Merged
merged 2 commits into from
Aug 9, 2023

Conversation

weissi
Copy link
Member

@weissi weissi commented Jul 13, 2023

Motivation:

(companion PR to apple/swift-nio#2471)

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 single 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 singletons type which lazily provide a singleton EventLoopGroup and an NIOThreadPool is IMHO a much better answer.

Modifications:

  • Add NIOTSEventLoopGroup.singleton
  • Add NIOSingletons.transportServicesEventLoopGroup

Result:

@weissi
Copy link
Member Author

weissi commented Jul 13, 2023

This cannot be merged until apple/swift-nio#2471 has been released in NIO 2.57.0

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Just putting a hold on this for now.

weissi added a commit to apple/swift-nio that referenced this pull request Jul 27, 2023
### 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 `EventLoopGroup`s. Today, many of those users resort to creating (and often leaking) threads because it's simpler. Adding a `.globalSingle` static var which _lazily_ provides singleton `EventLoopGroup`s and `NIOThreadPool`s 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 in `Dispatch`'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 add `NIOSingleton`s 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: 
 - apple/swift-nio-transport-services#180
 - swift-server/async-http-client#697

### Modifications:

- Add the `NIOSingletons` type
- Add `MultiThreadedEventLoopGroup.singleton`
- Add `NIOThreadPool.singleton`

### Result:

- Easier use of NIO that requires fewer parameters for users who don't require full control.
- Helps with #2142
- Fixes #2472
- Partially addresses #2473
@weissi weissi force-pushed the jw-nio-singletons branch from dfba0e5 to a775e12 Compare July 27, 2023 10:56
@weissi weissi added the 🆕 semver/minor Adds new public API. label Jul 27, 2023
@weissi weissi requested a review from Lukasa July 27, 2023 10:57
@weissi weissi force-pushed the jw-nio-singletons branch from a775e12 to bf033da Compare July 27, 2023 10:57
@weissi weissi requested a review from glbrntt July 27, 2023 10:58
Copy link
Contributor

@glbrntt glbrntt 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 aside from one nit and the missing NIO release

@weissi weissi force-pushed the jw-nio-singletons branch 2 times, most recently from 520b4a6 to 1c2b27e Compare July 27, 2023 19:52
@weissi
Copy link
Member Author

weissi commented Aug 8, 2023

With the NIO 2.58.0 release this should now go green

@weissi weissi force-pushed the jw-nio-singletons branch from 64b8f90 to 944225e Compare August 8, 2023 13:13
@weissi
Copy link
Member Author

weissi commented Aug 8, 2023

@Lukasa merging is currently blocked because it still has you down as a 'request changes', at your convenience PTAL

}

@available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *)
extension EventLoopGroup where Self == NIOTSEventLoopGroup {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this extension ever exist where the other does not?

Copy link
Member Author

@weissi weissi Aug 8, 2023

Choose a reason for hiding this comment

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

@Lukasa , @glbrntt suggested this one. The answer is yes, in the many places where we take any EventLoopGroup. there you can now type .singletonMultiThreadedEventLoopGroup/.singletonTransportServicesEventLoopGroup.

I don't know if I love it but NIO released this pattern and even if we decided that it's bad, it's an easy one to deprecated. Given that it's in NIO we should also have it in TS.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW I think this is the more useful API for most users because it's easy to discover: IDEs will suggest this when you dot-tab-complete for any EventLoopGroup. If you didn't have this API users would have to know about the ELG types or singleton objects to find them.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's deeply weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we have SwiftUI to thank for this 😃 e.g. https://developer.apple.com/documentation/swiftui/view/labelstyle(_:)

@weissi weissi requested a review from Lukasa August 8, 2023 16:13
@weissi weissi enabled auto-merge (squash) August 8, 2023 16:36
@weissi weissi merged commit 39ece4e into apple:main Aug 9, 2023
@weissi weissi deleted the jw-nio-singletons branch August 9, 2023 11:46
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.

4 participants