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

Use EventLoop provided by SwiftNIO's MultiThreadedEventLoopGroup.singleton #389

Merged
merged 8 commits into from
Aug 9, 2023

Conversation

tkrajacic
Copy link
Sponsor Contributor

@tkrajacic tkrajacic commented Aug 8, 2023

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.

// Uses the MultiThreadedEventLoopGroup.singleton.any() eventLoop
let connection = try await PostgresConnection.connect(
  configuration: config,
  id: 1,
  logger: logger
)

Fixes #388

Copy link
Collaborator

@fabianfett fabianfett left a comment

Choose a reason for hiding this comment

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

Two small nits.

README.md Outdated Show resolved Hide resolved
Sources/PostgresNIO/Connection/PostgresConnection.swift Outdated Show resolved Hide resolved
tkrajacic and others added 3 commits August 8, 2023 17:58
Co-authored-by: Fabian Fett <fabianfett@apple.com>
Co-authored-by: Fabian Fett <fabianfett@apple.com>
@fabianfett fabianfett added the semver-minor Adds new public API. label Aug 8, 2023
/// - 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,
Copy link
Contributor

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?

Copy link
Collaborator

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.

Copy link
Contributor

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.

Copy link
Collaborator

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?

Copy link
Collaborator

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.

Copy link
Sponsor Contributor Author

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tkrajacic sounds great!

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2023

Codecov Report

Merging #389 (352965b) into main (220eb50) will decrease coverage by 0.04%.
Report is 1 commits behind head on main.
The diff coverage is 0.00%.

❗ Current head 352965b differs from pull request most recent head 6eba762. Consider uploading reports for the commit 6eba762 to get more accurate results

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     
Files Changed Coverage Δ
...es/PostgresNIO/Connection/PostgresConnection.swift 34.87% <0.00%> (-0.68%) ⬇️

Copy link
Collaborator

@fabianfett fabianfett left a 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

Sources/PostgresNIO/Connection/PostgresConnection.swift Outdated Show resolved Hide resolved
@fabianfett fabianfett requested a review from gwynne August 9, 2023 14:40
@fabianfett fabianfett enabled auto-merge (squash) August 9, 2023 21:13
@fabianfett fabianfett merged commit a5758b0 into vapor:main Aug 9, 2023
12 checks passed
@fabianfett fabianfett added this to the 2.0.0 milestone Aug 9, 2023
@tkrajacic tkrajacic deleted the tk-allow-singleton-eventloop branch August 10, 2023 07:49
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.

Allow PostgresConnection creation without specifying an EventLoop
5 participants