Skip to content

Add new HTTPConnectionPool Manager #420

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 5 commits into from
Sep 10, 2021

Conversation

fabianfett
Copy link
Member

Follow up to #418

@fabianfett fabianfett requested a review from glbrntt September 10, 2021 10:26
final class Manager {
private typealias Key = ConnectionPool.Key

enum State {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: private

}

deinit {
guard case .shutDown = self.state else {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about shutting down? You could call shutdown and not bother waiting for the future to complete as well, I assume?

}
}

func shutdown(on eventLoop: EventLoop) -> EventLoopFuture<Bool> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the Bool here?

self.state = .shutDown
return .done(future)
} else {
let promise = eventLoop.makePromise(of: Bool.self)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Worth a comment here to indicate when this promise is completed.


case .shuttingDown(let promise, let soFarUnclean):
guard self._pools.removeValue(forKey: pool.key) === pool else {
preconditionFailure("Expected that the pool was ")
Copy link
Collaborator

Choose a reason for hiding this comment

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

That suspense is killing me!

Comment on lines 30 to 33
let eventLoopGroup: EventLoopGroup
let configuration: HTTPClient.Configuration
let connectionIDGenerator = Connection.ID.globalGenerator
let logger: Logger
Copy link
Collaborator

Choose a reason for hiding this comment

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

private?

@fabianfett fabianfett requested a review from glbrntt September 10, 2021 14:44
Copy link
Collaborator

@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.

LGTM

@fabianfett fabianfett merged commit fb49c1b into swift-server:main Sep 10, 2021
@fabianfett fabianfett deleted the ff-pool-manager branch September 10, 2021 15:25
@fabianfett fabianfett added this to the HTTP/2 support milestone Sep 10, 2021
@fabianfett fabianfett added the 🔨 semver/patch No public API change. label Sep 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants