fix(firestore): use pooled HTTP/2 connections by default to fix slow/failing concurrent requests#305
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a custom HTTP/2 client pool (Http2ClientPool) to multiplex Firestore requests over shared HTTP/2 connections in production, bringing the Dart SDK closer to the Node.js implementation. While the addition of HTTP/2 support is a great performance improvement, several critical issues were identified in the review: HTTP/2 header keys must be lowercased to prevent protocol errors; the statusCompleter and bodyController must be properly closed on stream completion or error to prevent hanging requests; socket leaks must be avoided by destroying the socket if HTTP/2 negotiation fails; and failed connections must be cleaned up from the pool to prevent memory leaks.
…ete ClientPool Node parity
…he transport and pool shutdown
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces HTTP/2 connection pooling by default for the Firestore client to address slow and failing requests under high concurrency. It implements a custom ClientPool and Http2ClientPool to manage and reuse HTTP/2 connections, and adds corresponding unit tests. The reviewer provided several actionable suggestions to improve robustness and performance: linking the cancellation of the response stream to the underlying HTTP/2 stream subscription to prevent resource leaks, wrapping resource cleanup in a try-catch block to handle failures safely, running connection collection unawaited to avoid adding latency to requests, and properly awaiting the termination of the connection pool during client shutdown to prevent hangs.
| if (_terminated) { | ||
| _maybeCompleteDrain(); | ||
| } else { | ||
| await _collectIfIdle(pooled); | ||
| } |
There was a problem hiding this comment.
Awaiting _collectIfIdle inside the finally block blocks the completion of the run call (and thus the user's Firestore request) until the idle connection is fully destroyed. Since connection cleanup is a background task, we should run it unawaited to avoid adding unnecessary latency to the user's request.
| if (_terminated) { | |
| _maybeCompleteDrain(); | |
| } else { | |
| await _collectIfIdle(pooled); | |
| } | |
| if (_terminated) { | |
| _maybeCompleteDrain(); | |
| } else { | |
| unawaited(_collectIfIdle(pooled)); | |
| } |
…ke pool shutdown awaitable, guard cleanup errors
Coverage Report✅ Coverage 74.86% meets 40% threshold Total Coverage: 74.86% Package Breakdown
Minimum threshold: 40% |
…tion, WIF/OIDC) away from the HTTP/2 pool
…ng WIF/OIDC credential negotiation
…d _dial plain instance methods
|
Would it make sense for the |
|
@brianquinlan this makes sense, do we land this first and open a new issue/PR on |
|
I would love to see a PR on the dart-lang/http repo. I'm going to be out next week so maybe @natebosch can review |
Fixes #297.
Dart's Firestore client used
dart:io'sHttpClient, which only speaks HTTP/1.1 — meaning every concurrent request opened its own TCP+TLS connection. At high concurrency (thousands of simultaneous requests), this caused connection-storm failures (ClientException,HandshakeException) and made Dart significantly slower than Node, whose Firestore client multiplexes many concurrent operations over a small pool of HTTP/2 connections.This PR makes pooled HTTP/2 the default transport for all production Firestore traffic, with no configuration required:
ClientPool<T>, mirroring nodejs-firestore'sClientPool(dev/src/pool.ts): packs load onto the most-full connection under a 100-stream cap (matching Node's default) rather than spreading evenly, opens a new connection once existing ones are full, stops routing new work to a connection once it's shown a connection-level failure, garbage collects idle connections past a configurable threshold, and drains in-flight operations gracefully before shutting down.Http2ClientPoolwrapsClientPool<ClientTransportConnection>as a plainhttp.BaseClient;googleapis_auth's existing token-refresh logic wraps it viabaseClient:, so credential handling is unchanged.Benchmark