Repository navigation
http-transport: cork backpressure, x-forwarded-proto precedence, payload 400, GET Accept - #267
Conversation
…payload 400, GET Accept
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe HTTP transport now honors forwarded URL protocols, handles uWS writable backpressure and aborts during chunked streaming, negotiates GET ChangesHTTP transport behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant NodeRuntime
participant ReadableStream
participant UwsResponse
Client->>NodeRuntime: request with protocol and streamed response
NodeRuntime->>ReadableStream: read chunk
NodeRuntime->>UwsResponse: cork and write chunk
UwsResponse-->>NodeRuntime: backpressure status
UwsResponse->>NodeRuntime: onWritable
NodeRuntime->>ReadableStream: resume reading
UwsResponse->>NodeRuntime: abort response
NodeRuntime->>ReadableStream: cancel body
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/http-transport/src/runtimes/node.ts`:
- Around line 24-26: The streaming response flow must cancel a pending reader
when the client aborts, not only wake waitWritable(). Extend UwsResponse with
the appropriate abort event hook, register it in the stream pump around the
reader.read() loop, and cancel the reader on abort so a stalled read exits and
cleanup completes. Add coverage for an abort occurring while reader.read() is
pending.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8fe32161-027a-4ea1-8c8d-bf4ed55b23bf
📒 Files selected for processing (4)
packages/http-transport/src/runtimes/node.tspackages/http-transport/src/server.tspackages/http-transport/tests/node-runtime.spec.tspackages/http-transport/tests/server.spec.ts
Closes #204, closes #213
runtimes/node.ts):res.cork(cb)returns the response object (always truthy), notwrite()'s boolean, so the backpressure wait was dead code and uWS buffered every un-drained byte of a streamed response in memory. Beyond the issue's sketch, uWS turned out to honor only the firstonWritableregistration per response — the old per-wait re-registration could never have worked, and the naive capture-only fix would deadlock on the second backpressure event. The pump now uses a single lazily-registeredonWritablehandler dispatching drain events to the current waiter, andonAbortedwakes a pending waiter so a client disconnect mid-backpressure exits the pump instead of leaking it. Verified against a live uWS server (slow client pauses the source at ~11/100 chunks, resumes to full delivery) plus deterministic unit tests over a scripted response double (two wait/drain cycles, drain with no waiter, abort-while-waiting, drain-then-abort).x-forwarded-protoprecedence (runtimes/node.ts):header || tls ? 'https' : 'http'parsed as(header || tls) ? …, so any header value — includinghttp— producedhttps. The header value now decides when present, falling back to the TLS param.?payload=returns 400 (server.ts): theJSON.parseSyntaxErrorfell into the unknown-error branch and surfaced as 500InternalServerError; it now maps toBadRequest.Accept(server.ts): GET responses unconditionally forced the default format; the client's Accept header is now negotiated, falling back to*/*only when negotiation fails (tolerating browser-navigation Accept headers).Summary by CodeRabbit
x-forwarded-proto(fallback to listener protocol when missing).Acceptnegotiation forGETrequests, using the client’sAcceptwhen appropriate and safely falling back when unsupported.GETpayloadquery parameters now returns HTTP 400 with a clear “Invalid payload” error.