-
-
Notifications
You must be signed in to change notification settings - Fork 79
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
Use EventLoop provided by SwiftNIO's MultiThreadedEventLoopGroup.singleton #389
Conversation
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.
Two small nits.
EventLoopGroup sIngletons were introduced in apple/swift-nio#2471
Co-authored-by: Fabian Fett <fabianfett@apple.com>
Co-authored-by: Fabian Fett <fabianfett@apple.com>
/// - logger: A logger to log background events into | ||
/// - Returns: An established ``PostgresConnection`` asynchronously that can be used to run queries. | ||
public static func connect( | ||
configuration: PostgresConnection.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're still figuring out the patterns here so I don't know if this is a good suggestion but maybe a defaulted parameter on eventLoop: EventLoop = MultiThreadedEventLoopGroup.singleton.any()
could work?
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.
Not sure about this in the context of NIOTS... If we can import NIOTS, I would love to use the NIOTS EventLoop in the future for those cases. Defaulting inside the API might prove problematic then.
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 NIOTS is supported, I'd suggest to go down the route of my AHC PR: https://github.com/swift-server/async-http-client/pull/697/files#diff-914333008acc7684e7d98c700381a134ecdf49f095ce644e5898e857a16feabfR880
Then PostgresNIO could provide a PostgresNIO.defaultEventLoopGroup
which would return either MTELG.singleton
or NIOTSELG.singleton
. And apart from the default value nothing needs to change.
If you don't want to "leak" that we're using MTELG.singleton today, then I'd suggest to add the static var defaultEventLoopGroup
to PostgresNIO with this PR.
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.
@weissi Lovely! @tkrajacic can you take ti from here?
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.
@tkrajacic Just discussed this a bit further with @weissi:
extension PostgresConnection {
/// Returns the default `EventLoopGroup` singleton, automatically selecting the best for the platform.
///
/// This will select the concrete `EventLoopGroup` depending which platform this is running on.
public static var defaultEventLoopGroup: EventLoopGroup {
#if canImport(Network)
if #available(OSX 10.14, iOS 12.0, tvOS 12.0, watchOS 6.0, *) {
return NIOTSEventLoopGroup.singleton
} else {
return MultiThreadedEventLoopGroup.singleton
}
#else
return MultiThreadedEventLoopGroup.singleton
#endif
}
}
And then we should default the existing connect method to . defaultEventLoopGroup
, instead of creating a separate connect method.
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.
@fabianfett I have all that locally staged, but I was waiting until I could use the new NIOTS default eventloop group. So I am waiting for that to get released
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.
@tkrajacic sounds great!
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
- Coverage 46.02% 45.98% -0.04%
==========================================
Files 110 110
Lines 8833 8840 +7
==========================================
Hits 4065 4065
- Misses 4768 4775 +7
|
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.
Looks great! Thanks @tkrajacic
Since SwiftNIO provides a global EventLoopGroup singleton, you can now create a PostgresConnection without explicitly providing an EventLoop. In such a case, an eventLoop from this singleton will be used.
Fixes #388