feat(http2): add a pooled, multiplexed HTTP/2 http.Client - #1956
Conversation
| ) async { | ||
| if (!transport.isOpen) throw const _ConnectionClosedByPeer(); | ||
|
|
||
| final path = |
There was a problem hiding this comment.
AI tells me the path should default to / when empty.
| Header.ascii(':path', path), | ||
| // HTTP/2 requires lowercase header names (RFC 7540 8.1.2). | ||
| for (final entry in request.headers.entries) | ||
| Header.ascii(entry.key.toLowerCase(), entry.value), |
There was a problem hiding this comment.
Again an AI comment:
HTTP/1.x connection-specific headers (connection, keep-alive, proxy-connection, transfer-encoding, upgrade) are forbidden in HTTP/2 and can cause strict servers to reject the stream with a protocol error. Since Http2Client may wrap requests originating from general HTTP/1.1-oriented code, consider stripping any connection-specific headers (as well as host, since :authority is already sent).
| var _terminated = false; | ||
| Completer<void>? _drained; | ||
|
|
||
| /// The number of resources currently in the pool. For testing. |
| int get size => _resources.length; | ||
|
|
||
| /// The number of in-flight operations across every resource. For testing. | ||
| int get opCount => |
There was a problem hiding this comment.
Also testing only annotation then
| } | ||
|
|
||
| bool get _hasExcessIdleCapacity { | ||
| final idleCapacity = _resources.fold( |
There was a problem hiding this comment.
final idleCapacity = _resources.fold(
0,
(total, resource) =>
total + (resource.failed ? 0 : maxConcurrentOperations - resource.inFlight),
);
To not count failed resources, which technically have no idle capacity, into the sum
| } | ||
|
|
||
| void _maybeCompleteDrain() { | ||
| final drained = _drained; |
There was a problem hiding this comment.
Is this just for not having to use a !? Then rather use a case final maybe?
| Future<void> terminate() async { | ||
| for (final pool in _pools.values) { | ||
| await pool.terminate(); | ||
| } |
There was a problem hiding this comment.
AI:
After awaiting pool.terminate() for all entries in _pools, consider calling _pools.clear();. If someone calls client.terminate() and subsequently sends a new request, clearing _pools allows _poolFor() to spin up a fresh pool rather than throwing a StateError('This pool has already been terminated.') from the old pool. Alternatively, if Http2Client is intended to be permanently unusable after termination, adding a _closed = true flag at the top of send() would give a clearer error message.
| statusCompleter.completeError(error, stackTrace); | ||
| } | ||
| bodyController.addError(error, stackTrace); | ||
| if (!bodyController.isClosed) bodyController.close(); |
There was a problem hiding this comment.
Nit: Always use parentheses with ifs.
|
|
||
| /// The number of connections currently pooled, across every host. | ||
| int get connectionCount => | ||
| _pools.values.fold(0, (total, pool) => total + pool.size); |
There was a problem hiding this comment.
Nit: use .sum from https://pub.dev/documentation/collection/latest/collection/IterableIntegerExtension/sum.html here and elsewhere.
| Future<MultiProtocolHttpServer> _bind() => | ||
| MultiProtocolHttpServer.bind('localhost', 0, _serverContext()); | ||
|
|
||
| // test/certificates/server_chain.pem is long expired and self-signed, same |
There was a problem hiding this comment.
Hm, then we should probably regenerate it?
brianquinlan
left a comment
brianquinlan
left a comment
There was a problem hiding this comment.
I'll have more review feedback tomorrow. Also, this PR adds (preliminary) conformance tests: #1960
You might want to add them to this PR.
| /// networks - do not use it to accept arbitrary certificates in production. | ||
| class Http2Client extends BaseClient { | ||
| Http2Client({ | ||
| this.maxStreamsPerConnection = 100, |
There was a problem hiding this comment.
Should this default to SETTINGS_MAX_CONCURRENT_STREAMS send by the server?
3 participants
Closes #1385
Adds
Http2Client, a pooled, multiplexedhttp.Clientbacked by HTTP/2 connections, plus the genericClientPool<T>it's built on — fixesdart:io'sHttpClientopening one connection per concurrent request. Ported and generalized from a downstream implementation built for firebase/firebase-admin-dart#305.ClientPool<T>: most-full-first packing, idle GC, failure retirement, awaitable gracefulterminate()Http2Client: pools perhost:port, caps concurrent handshakes globally, multi-host safe (e.g. asgoogleapis_auth'sbaseClient)ClientPooland integration tests forHttp2Clientcovering multi-host pooling, connection-cap dialing, graceful terminate, and the peer-close retry