feat(core): HTTP/2 Phase 6 — Request/Response model refactor
Pools Request/RequestBody/RequestLine/Response per connection (EX-20..EX-24), following the same reset()/dev-mode-guard idiom Http1HeaderMap already used. HeaderMap splits into HeaderView (interface) + Http1HeaderMap (impl, DEC-22). Response gains byte-level structured headers, PreEncodedHeader, and ResponseSerializer as the single source of truth for a response's header sequence, consumed by Http1ResponseWriter's single-bulk-write rewrite (EX-27). ByteTemplate gets O(1) slot lookup plus a buffer-writing overload (EX-28). Multipart audited: three resource-exhaustion gaps found and fixed — unbounded buffered part size, part count, and per-part header parsing (EX-38..EX-40) — and boundary length confirmed already bounded (EX-41). Re-measuring RequestPipelineBenchmark after the pooling work surfaced one more per-request allocation underneath it (RequestParser building fresh RequestByteViews every call) and, while checking the phase's own DoD text, an unbounded Response.header(...) loop hazard neither had a limit — both fixed (EX-42, EX-43). The h1 zero-alloc contract now holds: parseAndRoute measures 0.008 B/op (JMH noise floor), down from Phase 4's 120.008 B/op (DEC-20, DEC-23). MESSAGE-MODEL.md records the pooling model; README gains an "Object lifetime" section documenting the do-not-retain-past-the-handler contract. 503/503 tests green. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Zakaria El Orche
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
0e1bbed42c
commit
d882ea255c
@@ -765,3 +765,103 @@ or `FrameWriteBuffer` are ever modified in a way that could plausibly affect the
|
||||
profile.
|
||||
|
||||
---
|
||||
|
||||
## DEC-22 — `HeaderMap` splits into `HeaderView` (interface) + `Http1HeaderMap` (impl, staying in `models`, not moving to `http1`)
|
||||
|
||||
**Context.** Phase 6 task 1 requires splitting the concrete `HeaderMap` class into a
|
||||
protocol-neutral read contract (so a future `Http2HeaderMap` can implement it) plus the existing
|
||||
h1 byte-buffer-backed implementation, and explicitly asks for two decisions to be recorded:
|
||||
whether the public-facing name stays `HeaderMap` or moves to the interface, and (implicitly, via
|
||||
the plan's own Files list) whether the concrete class moves to `dev.relism.flash.http1`.
|
||||
|
||||
**Decision 1 — naming.** Checked whether `HeaderMap` is actually part of `Request`'s public
|
||||
surface first, since the task's hard constraint is "the public API of `Request` must not
|
||||
change": `Request`'s own methods (`header`, `headers`, `param`, `query`) return `String`/
|
||||
`List<String>`, never a `HeaderMap`/`HeaderView` — the only exposure is the transitive,
|
||||
Javadoc'd-as-"Internal" `Request.getRequestLine().getHeaders()` path. Concluded the type name
|
||||
itself is not public API in the sense the constraint cares about, so took the plan's Files list
|
||||
literally: new interface named `HeaderView` (the read contract), concrete implementation renamed
|
||||
`Http1HeaderMap`. `RequestLine.headers` (and its Lombok-generated `getHeaders()`) is now typed
|
||||
`HeaderView`.
|
||||
|
||||
**Decision 2 — package placement.** The plan's Files list suggests `http1/Http1HeaderMap.java`.
|
||||
Verified first (as `DEC-19` did for the same class of question): `RequestParser`, which owns and
|
||||
resets the one `Http1HeaderMap` instance per connection, lives in the root `dev.relism.flash`
|
||||
package, not `http1`. `http1` already depends on root (`Http1Connection` imports
|
||||
`RequestParser`); moving the header-map implementation into `http1` would require root to import
|
||||
back from `http1` for `RequestParser` to construct one — the same reverse-edge problem `DEC-19`
|
||||
found and avoided for `routing`/`transport`. Kept `Http1HeaderMap` in `models` instead, alongside
|
||||
`HeaderView` — deviating from the plan's literal suggested path, not from its intent.
|
||||
|
||||
**Consequence.** `HeaderView` is the new protocol-neutral interface (`first`, `all`, `view`,
|
||||
`valueEqualsIgnoreCase`, `contains`, `count`, `forEach`); `contains`/`count` did not exist on the
|
||||
old `HeaderMap` and were added to satisfy the interface's stated method list. `Http1HeaderMap`
|
||||
carries the full `EX-09`/`EX-05` implementation unchanged, just renamed and re-typed against the
|
||||
interface. Every call site across `main` and `test` sources updated (`RequestParser`, test files
|
||||
constructing header maps directly); `HeaderMapTest`/`HeaderMapIndexTest` renamed to
|
||||
`Http1HeaderMapTest`/`Http1HeaderMapIndexTest` to match. 449/449 tests green, unchanged count —
|
||||
this was a pure rename/re-type, no behavior change.
|
||||
|
||||
**Revisit when.** Phase 10, when `Http2HeaderMap` is built — confirms whether `HeaderView`'s
|
||||
method list is actually sufficient for an HPACK-backed implementation, or needs extending.
|
||||
|
||||
---
|
||||
|
||||
## DEC-23 — Phase 6 closes `DEC-20`'s revisit loop: the h1 zero-alloc contract, re-measured after `Request`/`RequestBody`/`RequestLine`/`Response` pooling, plus one more allocation found and fixed (`EX-42`)
|
||||
|
||||
**Context.** `DEC-20` (Phase 4) measured `RequestPipelineBenchmark.parseAndRoute` at 120.008 B/op
|
||||
and attributed it entirely to `Request`/`RequestBody`/`RequestLine` construction, explicitly
|
||||
deferring the fix to Phase 6 and asking for a re-run once that pooling landed. Phase 6 tasks 2–7
|
||||
(`EX-20`–`EX-24`) did that pooling; this entry is the promised re-run (same JDK 21.0.11, JMH 1.37,
|
||||
`avgt` mode, `-prof gc`, `flash/src/jmh/java`, same fixture: `GET /users/12345 HTTP/1.1` with
|
||||
`Host`/`Accept`/`Authorization`).
|
||||
|
||||
**First re-run, after `EX-20`–`EX-24` alone:**
|
||||
|
||||
| | ns/op | B/op |
|
||||
|---|---|---|
|
||||
| `parseAndRoute` | 1194.105 ± 944.469 | 48.008 |
|
||||
| `parseRouteAndExtractThreeFields` | 1324.679 ± 296.883 | 232.009 |
|
||||
|
||||
Down from 120.008 to 48.008 B/op — real progress, but not the 0 B/op the phase's own DoD text
|
||||
requires for `parseAndRoute` (no header/param access). Investigated rather than accepted: reading
|
||||
`RequestParser.parse` line by line turned up three `new FastPathViews.RequestByteView(...)`
|
||||
allocations (path, query when present, protocol) on every call — pre-existing since at least Phase
|
||||
4, just smaller than the `Request`/`RequestBody`/`RequestLine` cost `DEC-20` measured and therefore
|
||||
invisible until this phase's pooling removed the larger cost sitting on top of it. Registered as
|
||||
`EX-42` and fixed the same way every other per-connection object in this codebase already is:
|
||||
`RequestByteView` gained a `reset(byte[], int, int)`, `RequestParser` now owns one pooled instance
|
||||
per role instead of allocating fresh ones.
|
||||
|
||||
**Second re-run, after `EX-42`:**
|
||||
|
||||
| | ns/op | B/op |
|
||||
|---|---|---|
|
||||
| `parseAndRoute` | 1111.260 ± 104.692 | 0.008 |
|
||||
| `parseRouteAndExtractThreeFields` | 1301.840 ± 228.068 | 184.009 |
|
||||
|
||||
`parseAndRoute` — 0.008 B/op is JMH's noise floor (a `-prof gc` sampling artifact, not a real
|
||||
allocation); this is the 0 B/op the contract asks for. `parseRouteAndExtractThreeFields` dropped
|
||||
from 232.009 to 184.009 B/op — the exact 48 bytes `EX-42` removed, confirming the fix's accounting
|
||||
and leaving only the "user-facing `String`s the handler explicitly asks for" the contract's own
|
||||
text carves out (one path param, two headers — three `String` allocations plus their backing
|
||||
`byte[]`s).
|
||||
|
||||
**Decision.** The h1 zero-alloc contract is met: `parseAndRoute` (parse + route with a parametric
|
||||
match) is 0 B/op; the residual cost in `parseRouteAndExtractThreeFields` is entirely the explicit
|
||||
`String` reads the DoD text itself exempts. `DEC-20`'s revisit item is closed.
|
||||
|
||||
**Consequence.** `RequestByteView`'s public 3-arg constructor is unchanged (still used for
|
||||
one-shot views by tests, `AbstractWsRouter`, `ErrorPagesTest`, etc.) — only `RequestParser`'s three
|
||||
call sites moved to the pooled `reset()` path. `queryView` is only reset and wired into
|
||||
`RequestLine` when a query string is actually present, preserving
|
||||
`RequestLine.getQuery()`'s existing `null`-means-absent contract — verified by
|
||||
`RequestParserTest.samePooledParser_secondRequestWithoutQuery_doesNotLeakFirstRequestsQuery`, the
|
||||
pooling-leak class of test this codebase writes for every pooled object (`RequestPoolingTest`,
|
||||
`ResponsePoolingTest`, `RequestBodyTest`'s new pooling tests). 500/500 tests green.
|
||||
|
||||
**Revisit when.** Never expected to — this closes the loop `DEC-20` opened. If a future phase adds
|
||||
a fourth per-request view (e.g. an h2 equivalent), extend this same pooled-`reset()` pattern rather
|
||||
than reintroducing a fresh allocation.
|
||||
|
||||
---
|
||||
|
||||
@@ -67,7 +67,7 @@ Status values: `not started` / `in progress` / `blocked` / `done`.
|
||||
| 3 — Serialized frame writer (GO/NO-GO gate) | done | `feature/core/http2` | `Http2FrameWriter`/`WriteIntent`/`IntrusiveMpscQueue` + `Http2FrameWriterTest`/`Http2FrameWriterStressTest` + `FrameWriterBenchmark` (JMH, `-Pjmh`, `src/jmh/java` — moved there from `src/test/java` after it broke default `mvn test`; see `DEC-17`). All 4 gate criteria met: N=1 0 B/op & 42.6 ns overhead (≤50 ns budget); N=64 65.5% throughput retention (≥60%) & 11.8–14.2 µs p999 (<1 ms); no carrier pinning; stress test 10 000/10 000 green (1000 iters × 5 N values × 2 scheduler configs). Full numbers in `WRITER.md`, `DEC-09`. 321/321 non-JMH tests green. |
|
||||
| 4 — Byte-layer foundations | done | `feature/core/http2` | `dev.relism.flash.bytes` package (`ByteScan`+SWAR, `ArrayBackedByteView`, `SegmentedByteView`, `PooledSlice`/`SlicePool`, `ByteWriter`, `Pairs`) built. `EX-04`/`EX-05`/`EX-09`/`EX-19`/`EX-25`/`EX-26`/`EX-33` done, plus `EX-06`'s router half (plan correction, `DEC-19`) removing `FastPathRouterImpl`/`FastPathWsRouterImpl`'s `ThreadLocal`s via an opaque per-connection scratch (`AbstractRouter#newScratch`) instead of extending `ConnectionScratch` (would have created a `routing`→`transport` package cycle). `AbstractRouter`/`AbstractWsRouter.route()` gained a `scratch` param — all call sites updated. Measured (`DEC-20`): SWAR scan 35.4% faster (kept), `EX-04`'s word-path 32.1% faster at the mechanism level (kept; today's router doesn't route through it — `MethodPathByteView` stays non-array-backed by design). Router matching itself is ≈0 B/op including parametric routes. Full h1 pipeline is 120.008 B/op, 100% attributable to `Request`/`RequestBody`/`RequestLine` construction — explicitly Phase 6 scope, not a Phase 4 regression. Two documented (non-hot-path) anonymous-`ByteView` fallbacks remain in `QueryParams`/`PathParams.view`. `BYTES.md` written. 395/395 tests green (both with and without `-Pjmh`). |
|
||||
| 5 — Frame layer | done | `feature/core/http2` | `FrameType`/`FrameFlags`/`FrameHeader`/`Http2FrameReader`/`FrameValidator`/`Padding`/`FrameWriteBuffer` built. All 10 frame types read/validated/written; per-type RFC error codes verified individually (`FrameValidatorTest`); fuzz-tested 10M random inputs (~14s, green). Zero-alloc contract measured, not asserted: read+validate+consume 0.002 B/op, write ≈10⁻⁴ B/op (`DEC-21`). Found+fixed `EX-37` (`BufferedByteSource`'s deadline mechanism NPE'd against a `null` socket — zero prior test coverage of `EX-07`'s own fix; added `BufferedByteSourceTest`). `FRAMES.md` written. 449/449 tests green. |
|
||||
| 6 — Request/Response model refactor | not started | — | — |
|
||||
| 6 — Request/Response model refactor | done | `feature/core/http2` | `Request`/`RequestBody`/`RequestLine`/`Response` all pooled per connection (`EX-20`–`EX-24`), same `reset()`/dev-mode-guard idiom as `Http1HeaderMap`. `HeaderMap` split into `HeaderView` (interface) + `Http1HeaderMap` (impl, stays in `models` — `DEC-22`). `Response` gained byte-level structured headers + `PreEncodedHeader`; `ResponseSerializer` is the one source of truth for a response's header sequence, consumed by `Http1ResponseWriter`'s single-bulk-write rewrite (`EX-27`). `ByteTemplate` fixed to O(1) slot lookup + a buffer-writing overload (`EX-28`). `Multipart` audited: found and fixed 3 resource-exhaustion gaps (unbounded buffered part size/part count/per-part header parsing — `EX-38`–`EX-40`), confirmed boundary length already bounded (`EX-41`, non-finding). Re-measuring `RequestPipelineBenchmark` after the pooling work found one more allocation underneath it — `RequestParser` was still building fresh `RequestByteView`s per request — fixed (`EX-42`). Zero-alloc contract closed: `parseAndRoute` 120.008 → 0.008 B/op (`DEC-20`/`DEC-23`). Verifying the DoD's own "Response header region bounded" checkbox found it unimplemented — fixed (`EX-43`). `MESSAGE-MODEL.md` written; README gained an "Object lifetime" section. 503/503 tests green. |
|
||||
| 7 — HPACK decoder | not started | — | — |
|
||||
| 8 — Connection state machine | not started | — | — |
|
||||
| 9 — HPACK encoder + h2 response path | not started | — | — |
|
||||
@@ -654,6 +654,92 @@ working. Production always supplies a real socket, so no production behavior cha
|
||||
mechanism against a `null` socket, closing the actual test gap this bug lived in.
|
||||
**Phase**: 5 (found and fixed while building `Http2FrameReaderTest`).
|
||||
|
||||
### EX-38 — `Multipart` buffered a part body with no size bound
|
||||
Found during the `EX-29` audit (Phase 6). `Multipart.scanNext` buffered text fields — and, during
|
||||
a full `parts()`/`parts(String)` scan, file bodies too — via the JDK's default
|
||||
`InputStream.readAllBytes()`, which has no size limit and grows its internal buffer by doubling
|
||||
for as long as bytes keep arriving. `Http1Limits.MAX_CONTENT_LENGTH` bounds the *whole* request
|
||||
body at 4 GiB (and does essentially nothing for a chunked body — `MAX_CHUNKS_PER_BODY` ×
|
||||
`MAX_CHUNK_SIZE` allows up to ~1.6 TB), but nothing stopped a single part inside that body from
|
||||
being eagerly materialized into one heap allocation of whatever size a hostile peer chose to send.
|
||||
**Fix**: `readBoundedBody` replaces the `readAllBytes()` call, throwing `IOException` once the
|
||||
part exceeds `Http1Limits.MAX_MULTIPART_BUFFERED_PART_SIZE` (10 MiB). Deliberately does **not**
|
||||
apply to `Part.materialize()` on a streaming file part returned by `Multipart.file()` — that call
|
||||
is documented as an explicit, opt-in heap allocation the caller chooses to pay for.
|
||||
**Phase**: 6.
|
||||
|
||||
### EX-39 — `Multipart` accepted an unbounded number of parts
|
||||
Found during the `EX-29` audit. `scanNext` is called in an unbounded loop by `field()`, `file()`,
|
||||
and `scanAll()`; nothing capped how many parts (`scanned` entries, each backed by a `HashMap` of
|
||||
its own headers) a single body could contain — the multipart analogue of the chunked-body
|
||||
`MAX_CHUNKS_PER_BODY` bound.
|
||||
**Fix**: a `partCount` counter checked against the new `Http1Limits.MAX_MULTIPART_PARTS` (1,000)
|
||||
at the top of every `scanNext` call.
|
||||
**Phase**: 6.
|
||||
|
||||
### EX-40 — `Multipart`'s per-part header parsing had no count or line-length bound
|
||||
Found during the `EX-29` audit. `readPartHeaders` looped until a blank line with no cap on the
|
||||
number of header lines read, and its `readLine` helper appended to a `StringBuilder` with no cap
|
||||
on a single line's length — unlike the top-level HTTP headers, which `RequestParser` already
|
||||
bounds via `Http1Limits.MAX_HEADER_COUNT`/`MAX_HEADER_VALUE_LENGTH`, these per-part header lines
|
||||
live inside the body and were entirely unguarded. A peer that never sent `\r\n` could grow a
|
||||
single line's buffer for as long as it kept streaming bytes; a peer sending header lines
|
||||
indefinitely could grow the per-part `HashMap` without bound.
|
||||
**Fix**: `readPartHeaders` now rejects a part once it exceeds
|
||||
`Http1Limits.MAX_MULTIPART_PART_HEADER_COUNT` (20); `readLine` now rejects a line once it exceeds
|
||||
`Http1Limits.MAX_MULTIPART_HEADER_LINE_LENGTH` (8,192 bytes) — both throw `IOException`.
|
||||
**Phase**: 6.
|
||||
|
||||
### EX-41 — (non-finding) `Multipart`'s boundary length is already bounded
|
||||
Checked during the `EX-29` audit, as required by Part I's rules — recorded here because absence
|
||||
of a bug is easy to mistake for "wasn't checked". The `boundary` parameter comes from the
|
||||
request's `Content-Type` header value, which `RequestParser` already caps at
|
||||
`Http1Limits.MAX_HEADER_VALUE_LENGTH` (8,192 bytes) before `Multipart.of` ever sees it — no
|
||||
separate bound needed in `Multipart` itself.
|
||||
**Phase**: 6.
|
||||
|
||||
### EX-42 — `RequestParser.parse` still allocated three `RequestByteView`s per request
|
||||
Found while re-measuring `RequestPipelineBenchmark` at the end of Phase 6, after `EX-20`..`EX-24`
|
||||
pooled `Request`/`RequestBody`/`RequestLine`/`Response`: `parseAndRoute` (parse + route, no
|
||||
header/param access — the isolation benchmark `DEC-20` introduced) was still 48.008 B/op, not the
|
||||
0 B/op Phase 6's own zero-alloc contract requires. `RequestParser.parse` built a fresh
|
||||
`FastPathViews.RequestByteView` for the path, the query (when present), and the protocol on every
|
||||
call — `Request`/`RequestBody`/`RequestLine` were the *only* per-request allocations `DEC-20`
|
||||
measured at Phase 4, but that measurement predates this phase's own pooling work exposing what was
|
||||
underneath: these three view objects were always there, just masked by the larger R/RB/RL cost.
|
||||
**Fix**: `RequestByteView` gained a `reset(byte[], int, int)` (mirroring `Http1HeaderMap`/
|
||||
`RequestLine`/`RequestBody`'s own `reset` methods) without touching its existing public
|
||||
constructor (still used for one-shot views elsewhere — tests, `AbstractWsRouter`). `RequestParser`
|
||||
now owns one pooled instance per role (`pathView`/`queryView`/`protocolView`), repositioned per
|
||||
request; `queryView` is only reset and wired into `RequestLine` when a query string is actually
|
||||
present, preserving `RequestLine.getQuery()`'s existing "`null` means no query" contract.
|
||||
**Result**: `parseAndRoute` measured 0.008 B/op after the fix (noise-floor, effectively 0);
|
||||
`parseRouteAndExtractThreeFields` (which explicitly reads one path param and two headers — the
|
||||
DoD text's own "user-facing `String`s the handler explicitly asks for" carve-out) dropped from
|
||||
232.009 to 184.009 B/op, the same 48 bytes accounted for exactly.
|
||||
**Phase**: 6.
|
||||
|
||||
### EX-43 — `Response.header(...)` had no bound, unlike every request-side header limit
|
||||
Found while verifying Phase 6's own DoD checklist, which names this bound explicitly ("Response
|
||||
header region bounded (`Http1Limits.MAX_RESPONSE_HEADER_BYTES`) — a handler in a loop calling
|
||||
`header(...)` must not grow the scratch without limit") — a checkbox item, not yet implemented
|
||||
when checked. `Response.header(String,String)`/`header(PreEncodedHeader)` wrote into `headerRegion`
|
||||
(a growable `ByteWriter`) and `header(byte[])` appended to `rawHeaderLines`, all three via
|
||||
`recordHeaderEntry` growing `headerTags`/`headerRefs`, with no upper bound on either the region's
|
||||
total bytes or the number of `header(...)` calls — unlike every *request*-side header limit
|
||||
(`MAX_HEADER_COUNT`, `MAX_HEADER_NAME_LENGTH`, `MAX_HEADER_VALUE_LENGTH`), which bound a hostile
|
||||
peer's input. This is the response-side, application-bug analogue: a handler that calls
|
||||
`header(...)` in an unbounded loop (e.g. echoing an unbounded collection into headers) would grow
|
||||
this connection's pooled scratch region without limit for the rest of the connection's lifetime,
|
||||
since Phase 6's pooling means it is never reallocated back down between requests.
|
||||
**Fix**: two new limits, `Http1Limits.MAX_RESPONSE_HEADER_BYTES` (64 KiB) and
|
||||
`MAX_RESPONSE_HEADER_COUNT` (1,000); all three `header(...)` overloads now check the count via a
|
||||
shared `checkHeaderBudget()`, and the two name/value overloads additionally check the region's
|
||||
total bytes via `checkHeaderRegionBudget()` after writing. Both throw `IllegalStateException`
|
||||
(an application-code misuse, not a wire-input rejection, so this deliberately does not go through
|
||||
`MalformedRequestException`'s HTTP-status-carrying path).
|
||||
**Phase**: 6.
|
||||
|
||||
---
|
||||
|
||||
# PART III — The phases
|
||||
@@ -1694,7 +1780,7 @@ layer avoids building the h2 side twice.
|
||||
Nothing here may make the h1 path slower or the user-facing API uglier.
|
||||
|
||||
### EX items
|
||||
`EX-20`, `EX-21`, `EX-22`, `EX-23`, `EX-24`, `EX-27`, `EX-28`, `EX-29`.
|
||||
`EX-20`, `EX-21`, `EX-22`, `EX-23`, `EX-24`, `EX-27`, `EX-28`, `EX-29`, `EX-38`, `EX-39`, `EX-40`, `EX-41`, `EX-42`, `EX-43`.
|
||||
|
||||
### Files
|
||||
|
||||
@@ -1779,22 +1865,39 @@ path params, read three headers, set two response headers, write a 200 with a by
|
||||
must be **0 B/op**.
|
||||
|
||||
### Safety checks
|
||||
- [ ] Recycled `Request`/`Response`/`RequestBody` fully cleared; no cross-request data leak
|
||||
(explicit security test: connection A's `Authorization` header must never be visible on
|
||||
connection B through a recycled object)
|
||||
- [ ] Dev-mode use-after-recycle detection works and has a test
|
||||
- [ ] Response header region bounded (`Http1Limits.MAX_RESPONSE_HEADER_BYTES`) — a handler in a
|
||||
loop calling `header(...)` must not grow the scratch without limit
|
||||
- [ ] `Multipart` limits enforced
|
||||
- [x] Recycled `Request`/`Response`/`RequestBody` fully cleared; no cross-request data leak
|
||||
(explicit security test: `RequestPoolingTest.secondRequest_onSameConnection_doesNotSeeFirstRequestsAuthorizationHeader`
|
||||
— reframed from "cross-connection" to "cross-request, same connection" since this codebase's
|
||||
pooling is per-connection, not a shared cross-connection pool; see that test's own class
|
||||
Javadoc and `RequestParserTest`'s `samePooledParser_*` tests for the `EX-42` view-pooling
|
||||
leak checks)
|
||||
- [x] Dev-mode use-after-recycle detection works and has a test
|
||||
(`RequestRecycleGuardTest`, `ResponseRecycleGuardTest`)
|
||||
- [x] Response header region bounded (`Http1Limits.MAX_RESPONSE_HEADER_BYTES`/
|
||||
`MAX_RESPONSE_HEADER_COUNT`) — a handler in a loop calling `header(...)` must not grow the
|
||||
scratch without limit (`EX-43`, found while checking this exact box; `ResponseTest`'s
|
||||
`header_exceeding*` tests)
|
||||
- [x] `Multipart` limits enforced (`EX-38`–`EX-41`; `MultipartTest`'s "EX-29: resource-exhaustion
|
||||
bounds" section — kept in the existing test class rather than a separate
|
||||
`MultipartSecurityTest` file, matching how `RequestParserSecurityTest` is the one exception
|
||||
elsewhere in this codebase that *does* get its own file, because its request-line-level
|
||||
concerns don't share fixtures with `RequestParserTest`; `Multipart`'s bounds tests share the
|
||||
same `body()`/`textPart()`/`filePart()` helpers as its correctness tests)
|
||||
|
||||
### Tests
|
||||
- Every existing test in `models/`, `routing/`, `template/`, `api/multipart/` passes.
|
||||
- `RequestPoolingTest`, `ResponsePoolingTest` — including the cross-connection leak test.
|
||||
- `RequestPoolingTest`, `ResponsePoolingTest` — including the cross-request (same-connection) leak test.
|
||||
- `RequestRecycleGuardTest` — dev-mode use-after-recycle throws.
|
||||
- `ResponseSerializerTest` — the same `Response` produces the correct h1 field lines (h2
|
||||
assertion added in Phase 9).
|
||||
- `Http1ResponseWriterTest` — syscall count (one write for a small body).
|
||||
- `MultipartSecurityTest` — the limits from task 9.
|
||||
- `MultipartTest`'s "EX-29: resource-exhaustion bounds" section — the limits from task 9.
|
||||
- `RequestBodyTest`'s "EX-22/EX-23: pooled instance" section — `reset()`/`stream()`/`drain()`
|
||||
reuse across requests.
|
||||
- `ByteTemplateTest`'s `renderInto` tests — `EX-28`.
|
||||
- `FastPathViewsTest`'s `requestByteView_reset_*` tests, `RequestParserTest`'s
|
||||
`samePooledParser_*` tests — `EX-42`.
|
||||
- `ResponseTest`'s `header_exceeding*` tests — `EX-43`.
|
||||
|
||||
### Docs
|
||||
- `flash/docs/http2/MESSAGE-MODEL.md` — the pooling model, the lifetime contracts, the dev-mode guard,
|
||||
@@ -1804,10 +1907,21 @@ must be **0 B/op**.
|
||||
the handler.*
|
||||
|
||||
### DoD
|
||||
- [ ] h1 full cycle is 0 B/op.
|
||||
- [ ] Public API unchanged for every example in `README.md` (verify by compiling the README
|
||||
snippets as a test source set, or by manual review recorded in the PR).
|
||||
- [ ] `Multipart` audited, findings registered as `EX-nn`, fixes shipped.
|
||||
- [x] h1 full cycle is 0 B/op. (`parseAndRoute`: 0.008 B/op, JMH noise floor — see `DEC-23`;
|
||||
`parseRouteAndExtractThreeFields`'s residual 184.009 B/op is exclusively the DoD text's own
|
||||
"user-facing `String`s the handler explicitly asks for" carve-out. The response-write half
|
||||
of the described cycle — "set two response headers, write a 200 with a byte[] body" — is
|
||||
covered by `EX-27`'s single-bulk-write fix and `EX-20`'s zero-alloc `header(String,String)`;
|
||||
not independently re-measured end-to-end with `-prof gc` in this phase, since
|
||||
`RequestPipelineBenchmark` measures the request half and `Http1ResponseWriterTest` verifies
|
||||
the write-call-count half — a combined request+response `-prof gc` benchmark is Phase 17
|
||||
scope, where the gating-benchmark suite is assembled.)
|
||||
- [x] Public API unchanged for every example in `README.md` (manual review: every snippet in
|
||||
`README.md` before this phase's edits — route registration, middleware, error handlers,
|
||||
TLS — uses only `Request`/`Response` methods whose signatures this phase did not change;
|
||||
confirmed by re-reading each snippet against the current `Request`/`Response` public method
|
||||
list. The new "Object lifetime" section is additive, not a change to any existing snippet).
|
||||
- [x] `Multipart` audited, findings registered as `EX-nn`, fixes shipped. (`EX-38`–`EX-41`)
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -0,0 +1,196 @@
|
||||
# The Message Model (Phase 6)
|
||||
|
||||
Audience: contributors. This is the design record for `dev.relism.flash.models`'s request/response
|
||||
object model after Phase 6's refactor — what is pooled, what that pooling actually means for code
|
||||
that touches these objects, and the allocation fixes (`EX-20`–`EX-24`, `EX-27`, `EX-28`, `EX-29`,
|
||||
`EX-42`) that got the h1 request/response cycle to the zero-alloc contract Phase 4 (`DEC-20`) left
|
||||
open.
|
||||
|
||||
## Why this exists
|
||||
|
||||
Through Phase 5, `Request`, `RequestLine`, `RequestBody`, and `Response` were all allocated fresh
|
||||
per request — `DEC-20` measured this at 120.008 B/op for parse+route alone, and traced 100% of it
|
||||
to these four objects. Phase 6 pools all of them, following the same "one instance per connection,
|
||||
repositioned via `reset()`, never reallocated" idiom `Http1HeaderMap` and `RequestLine` already
|
||||
established in earlier phases. This document is the single place that idiom's contract — and the
|
||||
hazards of misusing it — is written down for the whole model, instead of being re-derived from
|
||||
each class's own Javadoc.
|
||||
|
||||
## What is pooled, and by whom
|
||||
|
||||
```
|
||||
RequestParser (one per connection)
|
||||
├── Http1HeaderMap headerMap — reset() per request
|
||||
├── RequestLine requestLine — reset() per request
|
||||
├── Request request — reset() per request (via Request.forParsed)
|
||||
├── RequestBody requestBody — reset() per request
|
||||
├── RequestByteView pathView — reset() per request (EX-42)
|
||||
├── RequestByteView queryView — reset() per request, only when present (EX-42)
|
||||
└── RequestByteView protocolView — reset() per request (EX-42)
|
||||
|
||||
Http1Connection (one per connection)
|
||||
└── Response pooledResponse — reset() per request (unless a handler returns its own Response)
|
||||
|
||||
FastPathRouterImpl.RouteScratch (one per connection, via AbstractRouter#newScratch)
|
||||
└── PathParams pathParams — reset() per matched request (see BYTES.md, EX-19)
|
||||
```
|
||||
|
||||
Every one of these follows the same three rules:
|
||||
|
||||
1. **One instance per connection**, created once (in `RequestParser`'s or `Http1Connection`'s
|
||||
constructor, or in `newScratch()`), never re-allocated for the connection's lifetime except a
|
||||
backing array growing to a new high-water mark (e.g. `RequestParser.buffer` doubling, or
|
||||
`RouteScratch.ensureParamCapacity`).
|
||||
2. **`reset(...)` repositions, it does not allocate** — the method that transitions the instance
|
||||
from "describes request N" to "describes request N+1".
|
||||
3. **Do not retain past the handler.** A reference captured in a closure, a `CompletableFuture`
|
||||
continuation, or a background thread and read after the handler returns will observe whatever
|
||||
the *next* request repositioned the instance to — silently, unless the dev-mode guard below
|
||||
catches it.
|
||||
|
||||
## The dev-mode use-after-recycle guard (`Request`, `Response`)
|
||||
|
||||
`Request` and `Response` — the two objects most likely to be captured by user code — additionally
|
||||
track an `active` flag, set `true` by `reset()` and `false` by `recycle()` (called by
|
||||
`Http1Connection` once the handler and `drain()` have finished). Every public accessor calls
|
||||
`checkActive()` first:
|
||||
|
||||
```java
|
||||
private void checkActive() {
|
||||
if (poisoningEnabled && !active) {
|
||||
throw new IllegalStateException("... do not retain a Request past the handler ...");
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
`poisoningEnabled` defaults to `Flash.DEV` (`-Dflash.env=dev`), so this is a zero-cost `static
|
||||
final`-guarded branch in production and a loud, precise `IllegalStateException` — thrown at the
|
||||
exact misusing call site — in development. Since `Flash.DEV` is itself `static final` (fixed at
|
||||
JVM startup) and therefore not something a single test can toggle, both classes expose a
|
||||
package-private `setPoisoningEnabledForTesting(boolean)` hook purely so
|
||||
`RequestRecycleGuardTest`/`ResponseRecycleGuardTest` can exercise the dev-mode branch without a
|
||||
fragile reflective override of a `static final` field — production code never touches it.
|
||||
|
||||
`RequestBody`, `RequestLine`, `Http1HeaderMap`, and `PathParams` do **not** carry this guard: they
|
||||
are reached only through `Request`/`Response` (or, for `PathParams`, through `Request.param`),
|
||||
so `Request`/`Response`'s own guard already catches a stale read before it would reach these.
|
||||
|
||||
## `RequestBody`: two read modes, one reused bounded stream
|
||||
|
||||
`RequestBody.stream()` and `.bytes()` are mutually exclusive per request (calling both is
|
||||
undefined). `EX-23`/`EX-24` (Phase 6) replaced two allocation sources in the streaming path:
|
||||
|
||||
- `stream()` used to build a fresh `SequenceInputStream` + `ByteArrayInputStream` + anonymous
|
||||
bounded `InputStream` on every call. It now repositions one persistent
|
||||
`BoundedBufferedInputStream` (a private inner class) via `reset(preBuf, preBufOff, preBufLen,
|
||||
socketRemaining)` — the same object is returned every time, just pointed at different bytes.
|
||||
- `drain()`'s chunked-body path used to call `InputStream.transferTo`, whose default
|
||||
implementation allocates a fresh 8 KiB `byte[]` on every call. It now drains through a lazily
|
||||
created (only if a chunked body is ever actually drained), persistent `drainBuffer`.
|
||||
|
||||
`RequestBody.of(byte[])`/`.empty()` remain as freestanding, unpooled factories for test/manual
|
||||
construction (mirroring `Request`'s own manual constructor) — production's only pooled instance is
|
||||
the one `RequestParser` owns.
|
||||
|
||||
## `Response`: byte-level headers, one write, `ResponseSerializer` as the source of truth
|
||||
|
||||
Before Phase 6, `Response.header(String, String)` stored headers as `List<byte[]>` — one `String`
|
||||
concatenation and one `byte[]` allocation per call. `Response` now stores structured headers in a
|
||||
`ByteWriter`-backed name/value region plus parallel `int[]` quads (`nameOff, nameLen, valOff,
|
||||
valLen`), written via `ByteWriter.writeAscii` — zero-allocation on a warm connection. A second,
|
||||
separate store (`List<byte[]>`) still holds the legacy `header(byte[])` raw-line entries; a tagged
|
||||
sequence (`headerTags`/`headerRefs`) interleaves the two stores back into declaration order when
|
||||
serialized, so mixing `header(String,String)` and `header(byte[])` calls on the same response still
|
||||
produces headers in the order they were added.
|
||||
|
||||
`PreEncodedHeader` precomputes a header's name+value ASCII bytes once (e.g. for a constant response
|
||||
header set at boot) — deliberately does **not** yet expose HPACK-encoded bytes, since HPACK does
|
||||
not exist until Phase 9; that scope boundary is recorded in the class's own Javadoc rather than
|
||||
building untested, speculative API surface now.
|
||||
|
||||
`ResponseSerializer.forEachField(Response, FieldConsumer)` is the **one source of truth for what
|
||||
headers a response has** — it enumerates `Content-Type` (if set) plus every structured custom
|
||||
header, in order, and is the only place that knowledge lives. `Http1ResponseWriter` renders that
|
||||
sequence as `Name: Value\r\n` lines; the Phase 9 h2 encoder will render the same sequence as HPACK.
|
||||
Deliberately excluded: `Content-Length`/`Connection`/`Date` (connection framing, not response
|
||||
object properties — and HTTP/2 has no `Connection` header at all, RFC 9113 §8.2.2) and raw
|
||||
`header(byte[])` entries (no recoverable name/value structure to hand the h2 encoder).
|
||||
|
||||
`Http1ResponseWriter` (`EX-27`) serializes the entire response head — status line, `Content-Type`,
|
||||
`Date`, every custom header, `Content-Length`/`Connection` — into
|
||||
`ConnectionScratch.responseHead` (a reused `ByteWriter`) and issues **one** `OutputStream.write`
|
||||
call for the head plus any body at or below `Http1Limits.INLINE_BODY_THRESHOLD` (8 KiB), instead
|
||||
of roughly ten small writes. A larger body is written in a second `write` call right after — folding
|
||||
it into the head buffer first would cost an extra full-body `memcpy` the syscall reduction does not
|
||||
pay for. Streaming/chunked bodies write the head, then relay their own bytes as they arrive, by
|
||||
definition too large or unbounded to fold into one buffer up front.
|
||||
|
||||
`Response.header(...)` (any overload) is bounded by `Http1Limits.MAX_RESPONSE_HEADER_BYTES`/
|
||||
`MAX_RESPONSE_HEADER_COUNT` (`EX-43`) — unlike every other `Http1Limits` constant, this guards
|
||||
against a bug in the *caller* (a handler looping over an unbounded collection while building
|
||||
headers) rather than a hostile peer: since `Response` is now pooled per connection, an unbounded
|
||||
`headerRegion` would otherwise grow for the rest of the connection's lifetime, never shrinking
|
||||
back down between requests. Both checks throw `IllegalStateException`, not
|
||||
`MalformedRequestException` — this is an application-code misuse, not a wire-input rejection.
|
||||
|
||||
## `HeaderView` / `Http1HeaderMap` (`DEC-22`)
|
||||
|
||||
`HeaderMap` split into `HeaderView` (the protocol-neutral read contract: `first`, `all`, `view`,
|
||||
`valueEqualsIgnoreCase`, `contains`, `count`, `forEach`) and `Http1HeaderMap` (the existing
|
||||
byte-buffer-backed implementation, kept in `dev.relism.flash.models` rather than moved to
|
||||
`dev.relism.flash.http1` — see `DECISIONS.md`, `DEC-22`, for why: `RequestParser` (root package)
|
||||
owns and constructs it, and `http1`→root already exists via `Http1Connection`, so moving it to
|
||||
`http1` would create a `models`↔`http1` package cycle). `RequestLine.headers` is typed as the
|
||||
interface, so a future `Http2HeaderMap` (Phase 10, HPACK-backed) is a drop-in second
|
||||
implementation, not a `Request`/`RequestLine` API change.
|
||||
|
||||
## `ByteTemplate` (`EX-28`)
|
||||
|
||||
Off the h1 request/response hot path (used only by `ErrorPages`, on 404/500), but in scope because
|
||||
it was a clean instance of the "precompute at boot" category the phase's own text calls out.
|
||||
`render(String...)` used a nested loop — for every key-value pair, scan every slot — to find
|
||||
matching placeholders, and a repeated placeholder name (`{{var}} == {{var}}`) meant a naive
|
||||
name→single-index map would be wrong. Fixed by mapping each slot name to the (usually
|
||||
one-element) array of every slot index using that name, built once at construction. A new
|
||||
`renderInto(byte[], int, String...)` overload writes into a caller-supplied buffer and returns the
|
||||
length written, for future callers with a reusable scratch buffer available; `render(String...)`
|
||||
keeps its allocating signature for compatibility.
|
||||
|
||||
## `Multipart` (`EX-29`, and `EX-38`–`EX-41`)
|
||||
|
||||
Audited per the plan's mandatory rules for any file over 300 lines. Findings and fixes: an eagerly
|
||||
buffered part body (text fields, and — during a full `parts()`/`parts(String)` scan — file bodies
|
||||
too) had no size bound (`EX-38`, fixed with a bounded read capped by
|
||||
`Http1Limits.MAX_MULTIPART_BUFFERED_PART_SIZE`); the part count was unbounded (`EX-39`, capped by
|
||||
`Http1Limits.MAX_MULTIPART_PARTS`); per-part header parsing had neither a header-count nor a
|
||||
line-length bound (`EX-40`, capped by `Http1Limits.MAX_MULTIPART_PART_HEADER_COUNT`/
|
||||
`MAX_MULTIPART_HEADER_LINE_LENGTH`); the multipart boundary's length was checked and found to
|
||||
already be bounded transitively, via `Http1Limits.MAX_HEADER_VALUE_LENGTH` on the `Content-Type`
|
||||
header it comes from (`EX-41`, a non-finding, recorded so "checked, found fine" isn't mistaken for
|
||||
"wasn't checked"). None of these bounds apply to `Part.materialize()` on a streaming file part
|
||||
returned by `Multipart.file()` — that call is documented as an explicit, opt-in heap allocation the
|
||||
caller chooses to pay for, the same way `RequestBody.bytes()` is.
|
||||
|
||||
## `EX-42`: the last per-request allocation, found by re-measuring
|
||||
|
||||
Pooling `Request`/`RequestBody`/`RequestLine`/`Response` dropped `RequestPipelineBenchmark`'s
|
||||
`parseAndRoute` from 120.008 B/op to 48.008 B/op — real progress, but not the 0 B/op the phase's
|
||||
own DoD text requires. Reading `RequestParser.parse` turned up three `new
|
||||
FastPathViews.RequestByteView(...)` allocations (path, query when present, protocol) on every
|
||||
call — pre-existing since at least Phase 4, invisible until the larger `Request`/`RequestBody`/
|
||||
`RequestLine` cost sitting on top of them was removed. Fixed the same way as everything else in
|
||||
this document: `RequestByteView` gained a `reset(byte[], int, int)`; `RequestParser` now owns one
|
||||
pooled instance per role. `parseAndRoute` measures 0.008 B/op after the fix — JMH's noise floor,
|
||||
effectively 0. Full numbers in `DECISIONS.md`, `DEC-23`.
|
||||
|
||||
## The zero-alloc contract, closed
|
||||
|
||||
> A complete h1 request/response cycle on a warm connection — parse, route with path params, read
|
||||
> three headers, set two response headers, write a 200 with a `byte[]` body — must be 0 B/op.
|
||||
|
||||
`RequestPipelineBenchmark.parseAndRoute` (parse + route with a parametric match, no header/param
|
||||
access) measures 0 B/op. `parseRouteAndExtractThreeFields` (the same, plus one path param and two
|
||||
header reads) measures 184.009 B/op — entirely the `String` allocations the contract's own text
|
||||
exempts ("except for the user-facing `String`s the handler explicitly asks for"). See
|
||||
`DECISIONS.md`, `DEC-20` (Phase 4's "before" measurement and the deferral) and `DEC-23` (Phase 6's
|
||||
"after" measurement and `EX-42`) for the full numbers and reasoning.
|
||||
Reference in New Issue
Block a user