Repository navigation
feat(http2): configurable receive windows and frame size - #1999
Conversation
A client built on package:http2 moved at most 64 KiB per round trip: the connection receive window is fixed at the protocol's 65535 bytes, so streamWindowSize alone cannot raise throughput. With a 4 MiB body from a Go net/http server through 100 ms of added RTT, HTTP/1.1 (dart:io HttpClient) took about 350 ms and this package 7.1 s. - Settings.connectionWindowSize grants a larger connection window with a WINDOW_UPDATE on stream 0 after the initial SETTINGS. - Settings.maxFrameSize is sent as SETTINGS_MAX_FRAME_SIZE. - The initial streamWindowSize and maxFrameSize apply to this end's receive side from the moment they are sent: no stream exists yet and the peer reads the SETTINGS before any stream. Waiting for the ACK let a large frame on an early stream fail flow control, and capped every early response's first round trip at 64 KiB. - Above the default 65535-byte window, received data is acknowledged once per half window rather than once per DATA frame (the existing TODO in IncomingWindowHandler.dataProcessed); a default-sized window is still acknowledged at once. - The new settings are validated up front with the others. Defaults are unchanged. With streamWindowSize 6 MiB, connectionWindowSize 16 MiB and maxFrameSize 1 MiB, the same transfer takes about 340 ms (140 ms without added delay, against 155 ms for HTTP/1.1). test/flow_control_test.dart checks each behaviour frame by frame against a hand-driven peer; each test fails with its part removed.
- SettingsHandler applies the peer's SETTINGS_MAX_FRAME_SIZE, so frames sent to it use its limit, and rejects a value outside 16384..16777215 as a PROTOCOL_ERROR (RFC 9113 section 6.5.2). - The ACK of this end's own SETTINGS_INITIAL_WINDOW_SIZE no longer emits onInitialWindowSizeChange: only the peer's value sizes the send windows of open streams (RFC 9113 section 6.9.2). - connectionWindowSize must be at least 65535, the connection window's starting size; a smaller value was silently ignored. - IncomingWindowHandler.dataProcessed ignores 0 bytes, so an empty DATA frame never yields a WINDOW_UPDATE with an increment of 0 (RFC 9113 section 6.9).
mosuem
left a comment
There was a problem hiding this comment.
Note
This review was generated by an AI coding agent (Jetski) on behalf of @mosuem.
Thanks for the quick turnaround on the last round. I re-checked all four fixes in 53e7ee3 against the source (the peerSettings guard, SETTINGS_MAX_FRAME_SIZE on both the read and the write side, the connectionWindowSize range, the zero-byte guard) and they look right; dart analyze --fatal-infos, dart format and the full dart test run are clean for me locally too.
One remaining issue on the server side (inline on connection.dart), plus a few non-blocking nits. Two points that don't belong to a line:
- The title and description say "client", but
connectionWindowSize/maxFrameSizelive onSettings, so they apply toServerSettingsas well, and the pre-application in_setupConnectionruns for servers too. Worth reflecting in the title/CHANGELOG. - On whether the tuned values should become defaults: I'd keep the defaults unchanged in this PR and take that up separately once this has landed.
- _setupConnection pre-applies streamWindowSize only when it does not shrink the window. A client may send on a stream before it has read the server's SETTINGS (RFC 9113 section 3.4), within the default 65535 bytes (section 6.9.2); a server with a smaller window pre-applied tore such a connection down with FLOW_CONTROL_ERROR. A smaller window applies on ACK, as on master. Regression test from the review in flow_control_test.dart. - WINDOW_UPDATEs are batched from twice the default window, so a window just above 65535 is no longer acknowledged less often than the default. - flow_control_test.dart checks that a frame was not sent after a PING round trip instead of a wall-clock sleep. - CHANGELOG condensed to the package's style.
|
Thanks for the second round. Both general points are addressed: the title, description and CHANGELOG no longer say "client", as the settings live on |
mosuem
left a comment
There was a problem hiding this comment.
Note
This review was generated by an AI coding agent (Jetski) on behalf of @mosuem.
Thanks — d94afb5 checks out. Verified locally: dart format, dart analyze --fatal-infos and the full suite (254 tests) are clean; the new server test fails without the >= guard exactly as it should (Connection is being forcefully terminated. (errorCode: 3)), incoming-window-handler-batches-from-twice-the-default fails without the threshold change, and flow_control_test.dart with the PING barriers is green on 10 consecutive runs. Title, description and CHANGELOG read well now.
One correction on the test-coverage claim, not blocking:
-
the ACK of our SETTINGS never grows what we SEND on an open streamno longer exercises theif (peerSettings)guard in_modifySettings. Withtuned(6 MiB) the pre-application already sets_acknowledgedSettings.initialWindowSizeto 6 MiB at construction, so by the time the ACK arrivesdifferenceis 0 and the guard is moot. I removed theif (peerSettings)locally and the test passed 5/5. The guard still matters for a window below 65535, which is not pre-applied: without it the ACK would shrink the send window of an open stream by 64535. A variant that catches that — fails with the guard removed (Expected: <131070> Actual: <66535>), passes as is:variant
test('the ACK of our SETTINGS never changes what we SEND, also for a ' 'window below 65535 (which is not pre-applied)', () async { await connect(const ClientSettings(streamWindowSize: 1000)); peer ..sendSettings() ..send(_windowUpdate, 0, 0, [0x00, 0x10, 0x00, 0x00]); final stream = client.makeRequest([ Header.ascii(':method', 'POST'), Header.ascii(':path', '/upload'), Header.ascii(':scheme', 'http'), Header.ascii(':authority', 'peer'), ]); stream.sendData(Uint8List(200000), endStream: true); int sent() => peer.frames .where((f) => f.type == _data && f.stream == 1) .fold<int>(0, (n, f) => n + f.payload.length); await peer.until(() => sent() >= 65535); peer.ackSettings(); await peer.barrier(); await peer.barrier(); expect(sent(), 65535); // The peer grants the stream another default window: all of it must be // usable, i.e. the ACK of our own smaller window must not have shrunk // the send window. peer.send(_windowUpdate, 0, 1, [0x00, 0x00, 0xff, 0xff]); await peer.until(() => sent() > 65535); await peer.barrier(); await peer.barrier(); expect(sent(), 2 * 65535); });
Either replacing the existing test with it or adding it alongside would make "each test fails with its part removed" hold for this one too.
-
Nit: the doc on
Settings.streamWindowSizesays "Above 2 * 65535 bytes", while the code (and the CHANGELOG /dataProcessedcomment) batch from 2 × 65535 inclusive.
Otherwise this looks good to me.
With a window above 65535 the pre-application already makes the ACK's difference 0, so the test passed without the guard. It now uses a window below 65535, which is not pre-applied, and checks that a further stream WINDOW_UPDATE is fully usable after the ACK (fails without the guard: 66535 sent instead of 131070). Doc on streamWindowSize: batching starts from 2 * 65535, inclusive.
|
Thanks, both right. You're correct about the coverage claim: my earlier check removed the guard together with the pre-application, so it never isolated the guard. Fixed in aa701ae: the existing test is replaced by your variant (a window below 65535, then a further stream WINDOW_UPDATE that must be fully usable after the ACK). It fails 5/5 with |
Revisions updated by `dart tools/rev_sdk_deps.dart`. dartdoc (https://github.com/dart-lang/dartdoc/compare/5eb0f2c..e1859cf): e1859cfc Fri Oct 9 14:53:01 2026 +0200 Sarah Zakarias Link a bare prefix only when it maps to exactly one library (dart-lang/dartdoc#4295) c48f6fb9 Fri Oct 9 14:25:42 2026 +0200 Sarah Zakarias Resolve prefixes through the analyzer's PrefixElement.scopeLibraries (dart-lang/dartdoc#4294) 3a6297e2 Wed Oct 7 12:56:14 2026 +0200 Sarah Zakarias Require analyzer 14.5.0 and bump to 9.0.10 (dart-lang/dartdoc#4292) ecosystem (https://github.com/dart-lang/ecosystem/compare/16dca7a..622ef61): 622ef61 Mon Oct 12 00:23:27 2026 -0700 Kevin Moore fix(firehose): enforce -wip on published versions and tighten changelog checks (dart-lang/ecosystem#467) 5576a85 Wed Oct 7 09:29:44 2026 +0200 Moritz Support use-flutter option in groundskeeper workflow (dart-lang/ecosystem#444) http (https://github.com/dart-lang/http/compare/585d433..23f8574): 23f8574 Thu Oct 8 23:27:25 2026 +0700 Cong Bang feat(http2): configurable receive windows and frame size (dart-lang/http#1999) 18051a6 Thu Oct 8 10:39:38 2026 +0200 Moritz Enable Flutter in Groundskeeper workflow for Flutter packages (dart-lang/http#2004) native (https://github.com/dart-lang/native/compare/a2a6179..10515af): 10515afca Mon Oct 12 12:21:52 2026 +0200 Daco Harkes [native_toolchain_c] Do not pass linker flags when compiling static libraries (dart-lang/native#3731) native is held back at 10515af, before dart-lang/native#3728, because it breaks the dartdev `dart install` native assets tests. See dart-lang/native#3743. test (https://github.com/dart-lang/test/compare/d749b10..30acb69): 30acb69e Mon Oct 12 12:42:54 2026 -0700 Nate Bosch Time out browser loads just before the suite load timeout (dart-lang/test#2786) 597a0326 Mon Oct 12 12:22:53 2026 -0700 Kevin Moore Fix misplaced changelog entries and remove no-op health check (dart-lang/test#2789) 0b6efee8 Mon Oct 12 12:16:29 2026 -0700 Nate Bosch [checks] Collapse nested subjects in condition descriptions (dart-lang/test#2784) 17c98493 Mon Oct 12 12:08:23 2026 -0700 Nate Bosch Follow symlinks in precompiled browser test directories (dart-lang/test#2787) 1cc813ec Fri Oct 9 11:59:56 2026 -0700 Nate Bosch [checks] Join nested collapsed subjects with "that" (dart-lang/test#2783) ada7b950 Thu Oct 8 12:03:42 2026 -0700 Nate Bosch Prepare to publish (dart-lang/test#2780) c4b165b3 Wed Oct 7 11:29:08 2026 -0700 Kevin Moore [checks] Require package:meta ^1.17.0 (dart-lang/test#2779) 4a363d70 Tue Oct 6 10:46:31 2026 -0700 Nate Bosch Let Chrome choose its remote debugging port (dart-lang/test#2778) 465d66ab Tue Oct 6 09:35:22 2026 -0700 Nate Bosch Keep Chrome stderr in the browser output when debugging (dart-lang/test#2777) tools (https://github.com/dart-lang/tools/compare/a66d517..2b54c71): 2b54c71af Mon Oct 12 10:16:45 2026 +0200 dart-ecosystem-bot[bot] [boring] Update BoringSSL to 3fed4f20 (dart-lang/tools#2655) 2337f995a Sat Oct 10 03:11:40 2026 +0300 Hasan Armoush [json_rpc_2] Add strictProtocolChecks parameter to Client (dart-lang/tools#2555) c65c69ed1 Fri Oct 9 08:54:25 2026 +0200 Moritz [boring] Move package:boring to dart-lang/tools (2/4): FFI helpers from package:webcrypto (dart-lang/tools#2615) a04e45491 Thu Oct 8 21:07:18 2026 -0700 Kevin Moore chore: wire package:api_summary and package:string_scanner PR auto-labeler (dart-lang/tools#2652) 8bdd4b92c Thu Oct 8 09:20:48 2026 +0200 Morgan :) [code_builder] Allow built_collection 6.x. (dart-lang/tools#2645) c16040d35 Wed Oct 7 17:30:08 2026 -0700 Kevin Moore [api_summary] Sort primary barrel first and experimental entry points last (v1.2.0) (dart-lang/tools#2619) b2d0780db Wed Oct 7 11:27:16 2026 -0700 Kevin Moore [stream_channel] Require package:async ^2.6.0 (dart-lang/tools#2644) 3ab7bf9d0 Wed Oct 7 11:35:27 2026 +0200 Moritz [boring] Move package:boring to dart-lang/tools (1/4): vendored BoringSSL, bindings and source build (dart-lang/tools#2614) 29df6348b Tue Oct 6 15:21:44 2026 -0700 Kevin Moore feat(coverage): support web coverage reports in test_with_coverage (dart-lang/tools#2448) Regenerate pkg/{analyzer,analyzer_plugin,analyzer_testing}/api.txt. The rolled package:api_summary v1.2.0 (dart-lang/tools#2619) sorts the primary barrel first and other entry points after it, so entries in these files are reordered. The APIs themselves are unchanged. Change-Id: Ie8ff19c5a981ea8ecee8939c75457986a947e0a7 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/563220 Reviewed-by: Samuel Rawlins <srawlins@google.com> Auto-Submit: Nate Bosch <nbosch@google.com> Reviewed-by: Paul Berry <paulberry@google.com> Commit-Queue: Paul Berry <paulberry@google.com>
Fixes #1997.
A connection on
package:http2moved at most 64 KiB per round trip: theconnection receive window is fixed at the protocol's 65535 bytes, so
streamWindowSizealone cannot raise throughput. With a 4 MiB body from aGo
net/httpserver and 100 ms of added round-trip time,dart:io'sHTTP/1.1 client took ~350 ms and this package's client 7.1 s.
The new settings live on
Settings, so they apply to clients and serversalike:
Settings.connectionWindowSize(65535..2147483647) grants a largerconnection window with a WINDOW_UPDATE on stream 0 after the initial
SETTINGS.
Settings.maxFrameSizeis sent asSETTINGS_MAX_FRAME_SIZE.streamWindowSize, andmaxFrameSize, apply to thisend's receive side from the moment they are sent: a peer that has not read
them yet stays within the defaults, which they still allow. Waiting for the
ACK capped every early response's first round trip at 64 KiB. A smaller
streamWindowSizestill applies on ACK, since a client may send before ithas read the server's SETTINGS (RFC 9113 section 3.4).
window rather than once per DATA frame (the TODO in
IncomingWindowHandler.dataProcessed); smaller windows are acknowledgedat once, as before.
_validateSettings.Fixed on the way (from review): the peer's
SETTINGS_MAX_FRAME_SIZEishonored when sending and refused outside 16384..16777215; the ACK of this
end's own
SETTINGS_INITIAL_WINDOW_SIZEno longer resizes send windows; aWINDOW_UPDATE of 0 is never sent.
Defaults are unchanged; whether a client should default to larger values
can be taken up separately once this has landed. With
streamWindowSize6 MiB,
connectionWindowSize16 MiB andmaxFrameSize1 MiB the transferabove takes ~340 ms (~140 ms without added delay, against ~155 ms for
HTTP/1.1).
Tests:
test/flow_control_test.dartdrives a raw peer and checks eachbehaviour on the wire, using a PING round trip (not a sleep) wherever it
asserts that a frame was not sent;
settings_handler_test.dartandwindow_handler_test.dartcover the review fixes. Each test fails with itspart of the change removed.
dart analyze --fatal-infos,dart formatandthe full
dart testpass (254 tests).🤖 Generated with Claude Code