[HTTP2] Prepare migration actions #456
Conversation
| http2State: HTTP2StateMachine, | ||
| newHTTP1Connection: Connection | ||
| ) -> Action { | ||
| self.migrateFromHTTP1( |
There was a problem hiding this comment.
This is very confusing: why is the outer function called migrateFromHTTP2 and the inner one called migrateFromHTTP1?
Sources/AsyncHTTPClient/ConnectionPool/State Machine/HTTPConnectionPool+HTTP2Connections.swift
Show resolved
Hide resolved
| /// - starting: starting HTTP connections from previous state machine | ||
| /// - backingOff: backing off HTTP connections from previous state machine | ||
| mutating func migrateConnections( | ||
| mutating func migrateFromHTTP2( |
There was a problem hiding this comment.
No, good catch. It should be migrateFromHTTP1
| let request: HTTPConnectionPool.StateMachine.RequestAction | ||
| let connection: EstablishedConnectionAction | ||
|
|
||
| var asStateMachineAction: HTTPConnectionPool.StateMachine.Action { |
There was a problem hiding this comment.
This would normally be spelled as an initializer on the target type, which already exists. Do we need the helper?
There was a problem hiding this comment.
It's just a bit more convenient but I will remove it.
| case .closeConnection(let connection, let isShutdown): | ||
| return .migration( | ||
| createConnections: migrationAction.createConnections, | ||
| closeConnections: migrationAction.closeConnections + CollectionOfOne(connection), |
There was a problem hiding this comment.
I don't know if including CollectionOfOne is the most efficient way to achieve this append.
| case migration( | ||
| createConnections: [(Connection.ID, EventLoop)], | ||
| closeConnections: [Connection], | ||
| isShutdown: StateMachine.ConnectionAction.IsShutdown |
There was a problem hiding this comment.
In which case would we run into this? I guess if we are in http/1 and are closing and a new http/2 connection comes up we should just close the new connection. No migrations should occur if we are in shutdown.
dnadoba
force-pushed
the
dn-http-migration-action
branch
from
5908084 to
d9a1bb6
Compare
dnadoba
force-pushed
the
dn-http-migration-action
branch
from
89e35d0 to
17a1d0c
Compare
| if self.http2Connections!.isEmpty { | ||
| self.http2Connections = nil | ||
| } | ||
| if self.connections.isEmpty, self.http2Connections == nil { |
There was a problem hiding this comment.
Prefer && to , when we aren't doing conditional binding.
3 participants
Motivation
During migration from HTTP/1.x to HTTP/2 (and vice versa) we may need to close and/or create connections. We previously did not allow any actions during migration. This PR lifts that limitation.
Changes
.migrationcase toActionto allow all possible combinations of actions that can happen during migrationConnectionMigrationActionandEstablishedConnectionActionwhich can me combined to a "normal"ConnectionActionwith the static.combined(_:_)method onConnectionActionEstablishedActionin the private implementation to be able to combine it with aConnectionMigrationAction