Repository navigation
fix(firestore): use pooled HTTP/2 connections by default to fix slow/failing concurrent requests - #305
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
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.
demolaf
changed the title
|
/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.
Coverage Report✅ Coverage 74.12% meets 40% threshold Total Coverage: 74.12% Package Breakdown
Minimum threshold: 40% |
|
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 |
92ae533 to
38d8a53
Compare
…ete ClientPool Node parity
…he transport and pool shutdown
…ke pool shutdown awaitable, guard cleanup errors
…tion, WIF/OIDC) away from the HTTP/2 pool
…ng WIF/OIDC credential negotiation
…d _dial plain instance methods
… the vendored pool and transport
…erver) away from the HTTP/2 transport
04e6768 to
b752016
Compare
Fixes #297.
Dart's Firestore client used
dart:io'sHttpClient, which only speaks HTTP/1.1, so every concurrent request opened its own TCP+TLS connection. At high concurrency this caused connection-storm failures (ClientException,HandshakeException) and made Dart much slower than Node, whose Firestore client multiplexes concurrent operations over a small pool of HTTP/2 connections.Firestore traffic now goes over
package:http2'sHttp2Client, which packs requests onto shared connections under a 100-stream cap and dials another once those fill. It speaks only HTTP/2, so a small wrapper keeps the endpoints that are still plain HTTP on HTTP/1.1: the emulator,Settings.ssl: false, and the credential sourcesgoogleapis_authreaches through the same base client, such as the GCE metadata server.Benchmark