bottom shelf - #517
Conversation
kevmoo
commented
Apr 20, 2026
kevmoo
commented
- pkg:bottom_shelf
- classes final to start with
- constants!
- modern formatting!
- Modern lints!
- mono-repo!
github-actions
Bot
added
the
type-infra
![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 bottom_shelf, a high-performance Shelf adapter that interfaces directly with raw sockets. It features a custom HTTP/1.1 parser using byte slices, lazy header parsing, and a dedicated response serializer. The review feedback identifies several critical issues in the initial implementation, particularly regarding request body handling: the current logic fails to support bodies spanning multiple TCP chunks, incorrectly handles keep-alive connections when a body is present, and loses data during socket hijacking. There are also concerns about the FixedLengthBodyStream not properly draining for keep-alive or preserving pipelined data, and a recommendation to replace print statements with a proper logging mechanism.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces bottom_shelf, a high-performance Shelf adapter designed to interface directly with raw sockets. It features a custom byte-slice HTTP/1.1 parser, support for chunked and fixed-length request/response bodies, and various security mitigations. Review feedback identified several critical areas for improvement, including a potential integer overflow in chunked body parsing, a race condition in header slice management during asynchronous request dispatch, and multiple RFC 9112 compliance issues related to header whitespace handling, mandatory Host headers for HTTP/1.1, and URI construction.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces bottom_shelf, a high-performance Shelf adapter that interfaces directly with raw sockets. It features a custom HTTP/1.1 parser using byte slices, streaming controllers for fixed-length and chunked request bodies, and a response serializer supporting chunked encoding. The implementation includes comprehensive documentation and a robust test suite covering security and protocol compliance. Review feedback highlights several opportunities to improve RFC 9112 adherence, including stricter validation of the Host, Connection, and Transfer-Encoding headers, as well as more rigorous parsing of header keys and lines to mitigate potential request smuggling and injection vulnerabilities. Additionally, the laziness of the header mapping was noted as a potential performance bottleneck due to how the shelf.Request constructor interacts with it.
| bool get isChunked { | ||
| if (_cache case {_CacheKey.isChunked: final bool value}) return value; | ||
| for (var slice in _slices) { | ||
| if (slice.key.matches($Header.transferEncoding)) { | ||
| final value = slice.value.asString().toLowerCase(); | ||
| if (value.contains('chunked')) { | ||
| _cache[_CacheKey.isChunked] = true; | ||
| return true; | ||
| } | ||
| } | ||
| } | ||
| _cache[_CacheKey.isChunked] = false; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
According to RFC 9112 Section 6.1, if a Transfer-Encoding header field is present, chunked must be the final encoding. The current implementation uses value.contains('chunked'), which is too permissive and could be bypassed if other encodings are appended after chunked.
| CaseInsensitiveMap<List<String>> get _map { | ||
| if (_inner == null) { | ||
| final map = _inner = CaseInsensitiveMap<List<String>>(); | ||
| for (var slice in _slices) { | ||
| map | ||
| .putIfAbsent(slice.key.asString(), () => []) | ||
| .add(slice.value.asString()); | ||
| } | ||
| } | ||
| return _inner!; | ||
| } |
There was a problem hiding this comment.
The LazyByteHeaderMap is not truly lazy when used with package:shelf. The shelf.Request constructor immediately iterates over the provided headers to internalize them, which triggers the _map getter and thus the hydration of the entire map and all string allocations. This defeats the performance benefits of using byte slices. Consider if a different approach to header handling is possible or acknowledge this limitation in the documentation.
| (_buffer[start] == $Chars.sp || | ||
| _buffer[end - 1] == $Chars.sp)) { |
There was a problem hiding this comment.
The check for invalid whitespace in the header key should also include the horizontal tab character (HTAB, ASCII 9), as RFC 9112 forbids any whitespace between the field name and the colon.
| (_buffer[start] == $Chars.sp || | |
| _buffer[end - 1] == $Chars.sp)) { | |
| (_buffer[start] == $Chars.sp || _buffer[start] == 9 || | |
| _buffer[end - 1] == $Chars.sp || _buffer[end - 1] == 9)) { |
| static String _getStatusPhrase(int statusCode) => switch (statusCode) { | ||
| 200 => 'OK', | ||
| 201 => 'Created', | ||
| 204 => 'No Content', | ||
| 301 => 'Moved Permanently', | ||
| 302 => 'Found', | ||
| 304 => 'Not Modified', | ||
| 400 => 'Bad Request', | ||
| 401 => 'Unauthorized', | ||
| 403 => 'Forbidden', | ||
| 404 => 'Not Found', | ||
| 500 => 'Internal Server Error', | ||
| _ => 'Unknown', | ||
| }; |
There was a problem hiding this comment.
The list of status phrases is incomplete. While the status code is the primary indicator, providing 'Unknown' for common codes (e.g., 206 Partial Content, 405 Method Not Allowed, 429 Too Many Requests) is not ideal for client compatibility. Consider expanding this list or using a more comprehensive mapping.
- Created `pkgs/_shelf_compliance` package to run HTTP/1.1 compliance tests. - Vendored the `Http11Probe` tool as a git submodule in `vendor/Http11Probe`. - Implemented a generic test harness in `test/compliance_test.dart` that runs the probe by category and verifies against goldens. - Added aggressive pruning to the JSON reports (removing skips, summary blocks, durations, and dynamic flags like `doubleFlush`) and saved them directly as JSON arrays. - Created `tool/generate_summary.dart` to post-process JSON reports into a clean, deduplicated markdown summary file (`shelf_summary.md`). - Added `mono_pkg.yaml` and configured a separate `compliance` workflow in `.github/workflows/compliance.yml`. - Added `tool/run_tests_with_dotnet.sh` to pre-install the .NET 10 SDK on CI runners before running tests. - Documented the package in its own README and linked to the summary from the root README.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces the _shelf_compliance package, which provides HTTP/1.1 compliance and hardening tests for the shelf ecosystem using the Http11Probe tool. It includes echo server implementations, test runner logic, and baseline reports for various RFC compliance categories. Feedback suggests using request.headersAll instead of request.headers in the echo handler to ensure that multiple instances of headers, such as Cookie, are processed as distinct values rather than being automatically joined into comma-separated strings, which is critical for accurate compliance verification.
|
Here is the pivoted table with the rows combining Path and Compile Mode, showing how much faster
(Note: The percentages in parentheses show how much faster Key Takeaways:
|
* Optimize hex parsing. The biggest improvement comes from using a `const` string as table. That avoids (potentially) a lazy initialization check on accessing the `_charFlags`. Also, a constant string may have a fixed location in memory. Negating the "is valid hex" and putting the bit in bit 7 allows using `entry.toSigned(8)`, which a smart compiler should be able to turn into a single sign-extending opcode. (Using `(entry & 0x0F) - (entry & 0x80)` works too.) The `missing_whitespace_between_adjacent_strings` lint doesn't understand that not all strings are text. Maybe it shouldn't trigger for strings containing character escapes.
Relocate runZonedGuarded from per-request dispatch up to connection handler, eliminating 10,000+ Zone allocations and transitions per second under keep-alive loads while preserving async error teardown.
…zyByteHeaderMap Implement package:shelf Headers contract directly on LazyByteHeaderMap and add zero-allocation ASCII mixed-case matching to HeaderByteSlice, bypassing header table copying on shelf.Request construction.
… response buffers Cache RFC 1123 Date header string per second and combine HTTP status line, headers, and known response bodies into a single buffer, reducing outbound TCP socket write syscalls to 1 per response.
…alysis Document zero-copy parser state machine, keep-alive pipelined flow, benchmark improvements (+14% RPS), and future zero-allocation proposals.
- Bind benchmark servers to 127.0.0.1 explicitly ('localhost' resolved
to IPv6-only ::1, unreachable by common load tools)
- shelf_io_bench_server.dart: identical pipeline on shelf_io (port 8082)
- dart_io_bench_server.dart: raw dart:io ceiling, no shelf (port 8083)
Measured (AOT, CPU-pinned, ab -k -c 50): bottom_shelf ~53k RPS,
dart:io ~18k, shelf_io ~15.6k.
Doc refresh: 15 files / 655 lines -> 3 files / 377 lines, with all open
work tracked in one place.
- ROADMAP.md is now the single tracker: Phases 1-4 compressed to a
completed summary; new phases capture the 2026-07-06 review findings:
- Phase 5: correctness/security follow-ups (parser buffer-reuse data
leak and response header injection marked CRITICAL)
- Phase 6: performance work, ranked, with measure-first discipline
- Phase 7: API & compliance hygiene (carried from TODO.md)
- Phase 8: pkg:shelf upstream fast-path track with related issue links
- BENCHMARKS.md (new): measured results (bottom_shelf ~53k RPS, 3.4x
shelf_io, 2.9x raw dart:io; AOT, CPU-pinned, interleaved trials),
reproduction commands, and methodology rules
- PERFORMANCE_ANALYSIS.md: corrected overstated claims in place
(stress_tester RPS artifact, "zero-allocation" parser, "100%"
compatibility resting on shelf src/ imports)
- Deleted completed/stale docs: PHASE_*.md (8), THE_REST.md, TODO.md,
perf_ideas.md, review_review.md, stress.md,
test_complete_bottom_shelf.md
OPTIONAL changes: benchmarking (5 interleaved trials, AOT, CPU-pinned, ab -k -c 50) shows NO measurable throughput difference vs baseline (~51.5k vs ~51.3k RPS, inside the ±3% noise band). The only measured effect is ~3% less garbage: 152 vs 157 GC scavenges over 300k identical requests (reproducible across runs). Per-request allocation is not the bottleneck at this level; keep or drop these on code-quality grounds. - Skip the always-discarded first Uri.parse for origin-form request targets (the common case); non-origin-form requests take the exact old path - Set TCP_NODELAY on accepted sockets (no effect on loopback benchmarks; matters for latency on real networks) - Micro-fixes: byte-compare common methods instead of allocating a sublist view; const '1.1' fast path for HTTP/1.1 version; static identity function in TypedHeaders.host; shared empty Uint8List for bodiless requests; sync fast path around handler invocation; skip awaiting completed bodyDone; cache ErrorResponse.bytes; build _HttpConnectionInfo once per connection instead of per request
Profile-guided investigation (2026-07-07, idle machine, AOT, CPU-pinned, interleaved trials): ~60% of CPU is socket syscalls; the addressable costs are async/stream machinery and string/map churn, not the parser (1.9%). Five independent prototypes measured: sync buffered-body path +10.2%, byte-oriented serializer +7.3%, no per-response flush +5.6%, shelf Request tax removed +4.4%, fused header scan +2.9%. Combined: +29.9% (53.2k -> 69.0k RPS, 4.4x shelf_io) with 56% less GC. Effects are additive (predicted +30.4% vs measured +29.9%). - docs/PROFILE_2026_07.md: full report — CPU/allocation/syscall profiles, showdown results, per-prototype productionization notes and caveats - docs/prototypes/: the six measurement patches + README (explicitly NOT production-ready; P1/P5 skip header validation, P3 drops backpressure, P4 removes load-bearing URL validation) - docs/ROADMAP.md: Phase 6 re-ranked by measured data (parser rewrite demoted; landed items marked done with honest null results); Phase 8 updated with measured +14.6% shelf-side receipts and the Request.adapter validation lesson (cf. #369) - docs/BENCHMARKS.md: 2026-07-07 results section - tool/vm_profile.dart: dependency-free vm_service profiler client (CPU samples + allocation profile) used for the investigation
Compute content-length count/validity/value, transfer-encoding presence, chunked-ness, host + duplicate detection, and the Connection token in a single walk of the header slices in the TypedHeaders constructor, replacing ~8 separate walks and the per-request _cache map inserts. validateTransferEncoding() now early-outs when no TE header is present. Semantics preserved exactly: all Content-Length occurrences counted, every TE header still individually validated, first recognized Connection token wins (as before). Verified by the smuggling/robustness suites. Measured +2.9% RPS (54,688 vs 53,154 median of 5 interleaved trials); see docs/PROFILE_2026_07.md.
Rewrites RawShelfResponseSerializer to build response heads as bytes in a reusable scratch buffer (materialized to the heap before any await, since concurrent connections interleave at await boundaries): - const status-line and Connection/Transfer-Encoding byte constants; Date header cached as bytes per second - no StringBuffer -> toString -> utf8.encode round trip, no per-header toLowerCase()/join(), known headers matched by length + ASCII compare - content-length captured during the existing headersAll iteration; no more Message.contentLength calls (each one hydrated shelf's entire singleValues map per response) SECURITY: fixes response splitting / header injection. Header names must be RFC 9110 tokens; values reject NUL, CR, LF, and non-Latin-1 code units. Violations throw before any bytes reach the socket, so the connection error path returns a clean 500 with no partial response on the wire. Previously names/values were written verbatim. Behavior change: header values above U+00FF were previously UTF-8 encoded on the wire (invalid per RFC 9110 field-value grammar); they are now rejected with a 500. Prototype without validation measured +7.3% RPS (57,024 vs 53,154; see docs/PROFILE_2026_07.md). Adds test/response_header_validation_test.dart (8 cases: CRLF/LF/NUL/ non-Latin-1/name-injection rejection, Latin-1 pass-through, multi-value join).
HeaderByteSlices point into the parser's single reused buffer. reset() runs right after the response is written, and the next keep-alive request overwrites the buffer from index 0. A handler that retained the Request past response completion (post-response logging, unawaited analytics) and then lazily read an un-hydrated header would read ANOTHER request's bytes — a silent cross-request data leak. Fix by poisoning: a SliceBufferToken shared by all slices of one request is invalidated in reset(); asString/matches/matchesKey then throw StateError instead of reading the reused buffer. Chosen over force-hydrating every request's headers on reset so the lazy-header fast path (no string allocation for headers nobody reads) is preserved. The danger window is exactly "first read after the response is sent", which is never safe for a buffer-backed slice. Adds test/slice_invalidation_test.dart. Throughput unchanged within noise (the added validity check is one predictable branch).
The per-response `await socket.flush()` gated each pipelined keep-alive request on the OS write draining. Drop it in favor of a bounded-buffer policy: writeResponse now returns the byte count, the connection accumulates it, and flushes only once $Limit.flushThreshold (256 KB) has queued. This bounds a fast-handler/slow-client buffer to roughly the threshold plus one response while letting small responses pipeline without blocking. Unflushed bytes are still delivered — dart:io drains socket.add asynchronously without flush(); flush() only provides a completion signal. The non-keep-alive and error paths still flush via socket.close() / _flushCloseDestroy(). Verified by a new 50-response keep-alive delivery test in bottom_shelf_test.dart. Measured +6% RPS (61,954 vs 58,419 median of 4 interleaved trials), consistent with the +5.6% prototype in docs/PROFILE_2026_07.md.
Cumulative effect of the three landed perf commits (fused header scan, byte-oriented serializer, byte-threshold flush) vs branch start e7e8621: +16.2% RPS (52,881 -> 61,454 median) and -34% GC scavenges (157 -> 103), pure bottom_shelf with no pkg:shelf changes.
A client RST during the response flush/write raises an unhandled SocketException (errno 104) that crashes the single isolate. Found via the gcp-http-bench two-VM harness (wrk connection-count sweep sends an RST when recycling its pool between steps); ab -k on loopback never hit it. Roadmap item records the root cause (_flushCloseDestroy's unguarded socket.flush().then, vs the guarded read path) and the fix direction (catch ECONNRESET/EPIPE on write and destroy that connection).
…te path - Catch and ignore errors on `socket.done` in `_HttpConnection` to prevent unhandled write exceptions from escaping. - Guard `socket.flush()` and `socket.close()` in `_flushCloseDestroy` to avoid uncaught cleanup exceptions. - Silently swallow `SocketException` on general catches and `_handleAsyncError` by destroying the connection. - Guard `serverSocket.listen` and `_handleConnection` in `RawShelfServer` to handle early client disconnects during accept. - Add regression test for client connection resets during response streaming.
2 participants
