[HTTP2] integrate HTTP2StateMachine into HTTPConnectionPool.StateMachine
#462
Conversation
dnadoba
force-pushed
the
dn-http2-state-machine-integration
branch
from
866f570 to
32c4e7f
Compare
| http1.failedToCreateNewConnection( | ||
| error, | ||
| connectionID: connectionID | ||
| ) |
There was a problem hiding this comment.
super nit: maybe make this one line.
And I know you just kept my style ;)
| ) | ||
|
|
||
| let newConnectionAction = http1StateMachine.migrateFromHTTP2( | ||
| http2State: http2StateMachine, |
There was a problem hiding this comment.
What properties are needed from the h2 state machine. Could those be passed explicitly? I should have catched this earlier. We create a dependency cycle on the state machines here.
There was a problem hiding this comment.
http1Connections, connections and requests. The method which takes a HTTP2StateMachine:
is just a convenience overload for:
which takes everything as separate arguments. We can just remove the convenience overload if you want.
| idGenerator: self.idGenerator | ||
| ) | ||
| let migrationAction = http2StateMachine.migrateFromHTTP1( | ||
| http1State: http1StateMachine, |
There was a problem hiding this comment.
As above. Could we pass the needed properties explicitly?
- fix code formatting style - remove migration convience overload
fabianfett
left a comment
fabianfett
left a comment
There was a problem hiding this comment.
LGTM. Very excited about this!
Lukasa
left a comment
Lukasa
left a comment
There was a problem hiding this comment.
The PR as-is is good. I've left two notes I'd like to see addressed, but they don't have to be addressed in this PR.
| return self.nextActionForAvailableConnection(at: index, context: context) | ||
| if self.connections.hasActiveConnection(for: connection.eventLoop) { | ||
| guard let (index, _) = self.connections.failConnection(connection.id) else { | ||
| preconditionFailure("we connection to a connection which we no nothing about") |
There was a problem hiding this comment.
I don't know what this error message is trying to say.
|
|
||
| let needRequiredEventLoopConnection = | ||
| // 2. or if we have requests for a required event loop | ||
| !self.requests.isEmpty(for: eventLoop) && |
There was a problem hiding this comment.
The performance impact of this phrasing of this check is a bit unfortunate: we search self.requests and self.connections twice. Ideally we'd have a combined check for these so we can iterate these two only once.
There was a problem hiding this comment.
This is true for self.connections but we do not really search self.requests twice because the requests with required event loop and without required event loop live in separate properties inside of self.requests.
3 participants
Motivation
To support HTTP/2 we need to integrate the
HTTP2StateMachineinto the new connection pool.Changes
http2state case inHTTPConnectionPool.StateMachineremoveAll(_:)was the inverse of what we actually wanted) which resulted in duplicated connections and lost connections.Note that we should never hit the
HTTP2StateMachineor be in thehttp2state because HTTP/2 is still disabled.HTTPConnectionPool.StateMachine