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.

Sign up for GitHub

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

Jump to bottom

[HTTP2] Create new connections during migration if needed #459

Merged
merged 6 commits into from
Oct 27, 2021
Merged

[HTTP2] Create new connections during migration if needed #459

merged 6 commits into from
Oct 27, 2021

Conversation

dnadoba
Copy link
Collaborator

@dnadoba dnadoba commented Oct 13, 2021

Motivation

We potential need to create new connections during migration:

HTTP1 -> HTTP2:

  • we have a queued request with a required event loop but do not have a connection starting or backing off for that event loop.

HTTP2 -> HTTP1

  • same as with HTTP1 -> HTTP2
  • in addition, we may have queued request without a required event loop. We then want to start more connections until we have a connection for each queued request or reaching the user defined maximum of concurrent connections.

Changes

  • create connection during migration as described above
  • add tests

@Lukasa Lukasa added the semver/patch For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Oct 14, 2021
@fabianfett fabianfett added this to the HTTP/2 support milestone Oct 14, 2021
Lukasa
Lukasa reviewed Oct 14, 2021
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift Outdated
createConnections.append(
contentsOf: preferredEventLoopsOfPendingGeneralPurposeRequests.lazy
/// we do not want to start additional connection for requests for which we already have starting or backing off connections
.dropFirst(self.startingGeneralPurposeConnections)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why are we using dropFirst? Is the order of objects in this collection semantic? We don’t use it anywhere else in this function so why are we throwing those away?

fabianfett
fabianfett reviewed Oct 14, 2021
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift Outdated
Comment on lines 544 to 568
self.connections.append(newConnection)
self.connections.insert(newConnection, at: self.overflowIndex)
self.overflowIndex = self.connections.index(after: self.overflowIndex)
Copy link
Member

Choose a reason for hiding this comment

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

I think we need more logic here! If we have 64 EventLoops and 64 requests that all require a different EL, we will blow through the default self.maximumAdditionalGeneralPurposeConnections without checking.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is correct, however we can't close starting connections because ClientBoostrap doesn't support it. What logic would you add instead?

fabianfett
fabianfett reviewed Oct 14, 2021
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift Outdated
Comment on lines 551 to 576
self.connections.append(backingOffConnection)
self.connections.insert(backingOffConnection, at: self.overflowIndex)
self.overflowIndex = self.connections.index(after: self.overflowIndex)
Copy link
Member

Choose a reason for hiding this comment

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

Same as above.

fabianfett
fabianfett reviewed Oct 14, 2021
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift Outdated
Comment on lines 579 to 593
// create new connections for requests with a required event loop
let eventLoopsWithConnectionThatCanOrWillBeAbleToExecuteRequests = Set(
self.connections.lazy
.filter {
$0.canOrWillBeAbleToExecuteRequests
}.map {
$0.eventLoop.id
}
)
var createConnections = requiredEventLoopsOfPendingRequests.compactMap { eventLoop -> (Connection.ID, EventLoop)? in
guard !eventLoopsWithConnectionThatCanOrWillBeAbleToExecuteRequests.contains(eventLoop.id)
else { return nil }
let connectionID = self.createNewOverflowConnection(on: eventLoop)
return (connectionID, eventLoop)
}
Copy link
Member

Choose a reason for hiding this comment

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

This is not in line, with how we currently process requests in http/1.1. For required eventLoops, we try to keep the number of starting (in this case meaning connecting and backingOff) connections the same as the number of queued requests.

@dnadoba dnadoba force-pushed the dn-http-migration-create-new-connections branch from e7d43f3 to b39c948 Compare October 26, 2021 14:16
@dnadoba
start new connections if needed during migration
195f852
@dnadoba
fix review comments
2eb2afc 
- new connections that are created after the http2 -> http1 migration should now more closely mimic the behaviour of the http1 state machine
- API bounderies are now clearer
@dnadoba dnadoba force-pushed the dn-http-migration-create-new-connections branch from b39c948 to 2eb2afc Compare October 26, 2021 14:17
@dnadoba dnadoba force-pushed the dn-http-migration-create-new-connections branch from 1585eac to 6b9d55b Compare October 26, 2021 15:37
fabianfett
fabianfett reviewed Oct 26, 2021
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift Outdated
Comment on lines 616 to 626

outerLoop: for (eventLoop, requestCount) in generalPurposeRequestCountGroupedByPreferredEventLoop {
let connectionsToStart = max(requestCount - unusedGeneralPurposeConnections, 0)
unusedGeneralPurposeConnections -= min(requestCount, unusedGeneralPurposeConnections)
for _ in 0..<connectionsToStart {
guard self.canGrow else {
break outerLoop
}
connectionToCreate.append((self.createNewConnection(on: eventLoop), eventLoop))
}
}
Copy link
Member

@fabianfett fabianfett Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is correct. However I think, we could make this code a little easier to read. Wdyt about creating a variable:

var neededGeneralPurposeConnections = max(requestCount - unusedGeneralPurposeConnections, 0)

and then decreasing the variable for each created connection? Break the loop as soon as the variable has reached 0.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure if I fully understand what you mean. It seams you want to replace the for loop in line 620 with a while loop and exit it if we hit zero, which is exactly the same behaviour as the for loop. I would argue that the intent of the for loop is clearer (and safer) compared to a while true loop with a break on zero.

Or do you first want to sum up the actually number of neededGeneralPurposeConnections regardless of event loop. Then we could replace the two for loops with only one while/for loop.

Copy link
Member

Choose a reason for hiding this comment

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

The later. :)

fabianfett
fabianfett reviewed Oct 26, 2021
View reviewed changes
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP1Connections.swift Outdated
Comment on lines 601 to 604
/// We need a connection for each queued request with a required event loop.
/// Therefore, we look how many request we have queued for a given `eventLoop` and
/// how many connections we are already starting on the given `eventLoop`.
/// If we have not enough, we will create additional connections to have at least on connection per request.
Copy link
Member

Choose a reason for hiding this comment

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

Is the /// intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have added three slashes in a couple of places. Do we have any style guide when to use three and when to use two? I slightly tend to like three slashes more because the non-monospaced font is a bit easier to read for me but I do not have a strong opinion on that topic.

Copy link
Member

Choose a reason for hiding this comment

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

I think we use three in documentation comments (like above a method) and two inline. At least that's what I've seen (and done) so far.

fabianfett
fabianfett approved these changes Oct 26, 2021
View reviewed changes
Copy link
Member

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

Number of small nits. But this a great patch.

@dnadoba dnadoba merged commit 4147fd6 into swift-server:main Oct 27, 2021
@dnadoba dnadoba deleted the dn-http-migration-create-new-connections branch November 4, 2021 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianfett fabianfett fabianfett approved these changes

@Lukasa Lukasa Lukasa approved these changes

Assignees
No one assigned
Labels
semver/patch For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Milestone
HTTP/2 support
Development

Successfully merging this pull request may close these issues.
3 participants
@dnadoba @Lukasa @fabianfett