Make http1Connections and http2Connections non-optional
#482
Conversation
| case .removeConnection: | ||
| return true | ||
| if index < initialOverflowIndex { | ||
| self.overflowIndex = self.connections.index(before: self.overflowIndex) |
There was a problem hiding this comment.
Previously we didn't update self.overflowIndex correctly which did hide some bugs because we rarely reused HTTP1Connections ever again after migration.
There was a problem hiding this comment.
Would love to have a comment like this here:
// If the connection has an index smaller than the previous overflow index, we deal with
// a general purpose connection. For this reason we need to decrement the overflow index.
Great catch!
There was a problem hiding this comment.
Also added to .shutdown() from where I have borrowed this code snippet.
| case .removeConnection: | ||
| return true | ||
| if index < initialOverflowIndex { | ||
| self.overflowIndex = self.connections.index(before: self.overflowIndex) |
There was a problem hiding this comment.
Would love to have a comment like this here:
// If the connection has an index smaller than the previous overflow index, we deal with
// a general purpose connection. For this reason we need to decrement the overflow index.
Great catch!
| @@ -523,7 +515,7 @@ extension HTTPConnectionPool { | |||
| // Connections report events back to us, if they are in a shutdown that was | |||
| // initiated by the state machine. For this reason this callback might be invoked | |||
| // even though all references to HTTP2Connections have already been cleared. | |||
| _ = self.http2Connections?.newHTTP2MaxConcurrentStreamsReceived(connectionID, newMaxStreams: newMaxStreams) | |||
| _ = self.http2Connections.newHTTP2MaxConcurrentStreamsReceived(connectionID, newMaxStreams: newMaxStreams) | |||
| @@ -532,26 +524,20 @@ extension HTTPConnectionPool { | |||
| // Connections report events back to us, if they are in a shutdown that was | |||
| // initiated by the state machine. For this reason this callback might be invoked | |||
| // even though all references to HTTP2Connections have already been cleared. | |||
| _ = self.http2Connections?.goAwayReceived(connectionID) | |||
| _ = self.http2Connections.goAwayReceived(connectionID) | |||
| self.http1Connections = HTTP1Connections( | ||
| maximumConcurrentConnections: maximumConcurrentHTTP1Connections, | ||
| generator: idGenerator | ||
| ) |
There was a problem hiding this comment.
I bet we reserve an array within this initializer. Maybe we want to add another initializer here, that really creates an empty HTTP1Connections. The struct is replaced in the migration anyway.
The constructor should set maximumConcurrentConnections to a default value. Then we don't need to pass it in the HTTP2 constructor.
Follow up from #481 (comment)