Skip to content

Implement asynchronous shutdown #183

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

Merged
merged 11 commits into from
Mar 30, 2020
Merged

Implement asynchronous shutdown #183

merged 11 commits into from
Mar 30, 2020

Conversation

artemredkin
Copy link
Collaborator

Motivation:
Most NIO-based client implementation provide asynchronous shutdown method, we need to provide one as well. Closes #126

Modifications:

  1. Converted all close and prepareClose methods in pool and connection providers to be asynchronous.
  2. Removed checks for not running close and prepareClose in pool and connection providers on event loop.
  3. Converted shutdown to use NIO asynchronous event loop shutdown method.
  4. Implemented synchronous shutdown methods on top of asynchronous ones.

Result:
Client of the library can now use asynchronous shutdown when required.

@artemredkin artemredkin requested review from weissi and Lukasa March 27, 2020 11:37
@artemredkin artemredkin added this to the 1.2.0 milestone Mar 27, 2020
@artemredkin artemredkin added the 🆕 semver/minor Adds new public API. label Mar 27, 2020
Copy link
Contributor

@weissi weissi 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 to me, just to nits regarding queues

/// callback instead of an EventLoopFuture. The reason for that is that NIO's EventLoopFutures will call back on an event loop.
/// The virtue of this function is to shut the event loop down. To work around that we call back on a DispatchQueue
/// instead.
public func shutdown(_ callback: @escaping (Error?) -> Void) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don’t default to .global and take a DispatchQueue instead. A user who wants async shutdown will know what queue they want to be called on and it’s very likely not going to be `.global

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure, one question is though: should EventLoopGroup do the same?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(done)

@weissi
Copy link
Contributor

weissi commented Mar 27, 2020

CC @adtrevor , mind taking a look at the new async shutdown?

@artemredkin
Copy link
Collaborator Author

@swift-server-bot test this please

1 similar comment
@artemredkin
Copy link
Collaborator Author

@swift-server-bot test this please

Copy link
Contributor

@weissi weissi left a comment

Choose a reason for hiding this comment

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

thanks, lgtm

Copy link
Contributor

@PopFlamingo PopFlamingo 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 to me! 🙂 Just a small note about an assertion.


return EventLoopFuture.andAllComplete(connectionProviders.map { $0.close() }, on: eventLoop).map {
self.connectionProvidersLock.withLock {
assert(self.connectionProviders.count == 0, "left-overs: \(self.connectionProviders)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not 100% sure: if the check is delayed there might be cases where the assertion would have triggered but does not anymore. Even if true, I don't know how much important it is because the order/synchronization of the close actions will need to be improved anyway as we are already aware of issues linked to this in #175 and #176

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I'll look into that when I'll get to those issues, thanks!

@artemredkin artemredkin merged commit a6d1ebe into master Mar 30, 2020
@artemredkin artemredkin deleted the async_shutdown branch March 30, 2020 12:23
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.

no asynchronous shutdown method
4 participants