Fix Content-Length piping RFC violation + optimize chunked mode syscalls#107
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
uWestJS Benchmark Results
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/http/core/response.ts (1)
197-213:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftBackpressure is dropped in the chunked-mode
_write()path, risking unboundedpendingChunks[]growth.
writeChunk()returnsfalsewhenflushChunks()hits uWS backpressure, but the chunked branch ignores that return value and unconditionally invokescallback(). The Writable above will keep handing more chunks down, each appended topendingChunks[], whileflushChunks()keeps failing untilonWritabledrains. Under a slow consumer this trades the old per-chunk syscall pressure for unbounded JS memory growth.Defer the Writable callback until backpressure clears (or batch within
writeChunkitself).♻️ Suggested approach
} else { - // Chunked mode: use writeChunk() batching for fewer syscalls - this.writeChunk(chunk, encoding); - callback(); + // Chunked mode: use writeChunk() batching for fewer syscalls + const ok = this.writeChunk(chunk, encoding); + if (ok || this.finished || this.aborted) { + callback(); + } else { + // Backpressure: wait for drain before signaling Writable readiness + this.uwsRes.onWritable(() => { + callback(); + return true; + }); + } }Note: the
onWritablehandler registered here will race with the onewriteChunkschedules forflushChunks(). Consider unifying drain handling so the callback is only resolved after pending chunks actually flush.🤖 Prompt for 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. In `@src/http/core/response.ts` around lines 197 - 213, The chunked branch of _write currently ignores writeChunk()'s boolean return and calls callback() immediately, which drops backpressure and allows unbounded growth of pendingChunks; change _write to check the return of writeChunk(chunk, encoding) and only call callback() when writeChunk returns true (or when backpressure clears), and if writeChunk returns false enqueue the callback (e.g., pendingWriteCallbacks) to be invoked from flushChunks() or the onWritable handler once pendingChunks have actually been flushed; unify the drain handling so both the writeChunk-scheduled flush and the onWritable handler resolve the same pending callback queue instead of racing.
🤖 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 `@src/http/core/response.ts`:
- Around line 342-349: The code that updates this.contentLengthTotal from the
Content-Length header (check involving lowerName === 'content-length') should
strictly validate the header value and clear the stored total on invalid input:
instead of using parseInt (which tolerates trailing junk) accept only a pure
non-negative integer string (e.g. /^\d+$/) derived from the last element when
value is an array, and if it fails validation set this.contentLengthTotal to
undefined/null (not leave the previous valid total); ensure this behavior is
applied wherever headers are normalized/overwritten (affecting writeHead() and
tryEnd() flows) so stale totals cannot survive an invalid overwrite.
- Around line 259-274: The _final() path that currently calls uwsRes.end() when
contentLengthTotal !== undefined must detect incomplete streams (i.e., when
tryEnd()/read tracking did not set this.finished because bytes <
this.contentLengthTotal) and not silently end; instead, if bytes are short,
invoke the Writable final callback with an Error (callback(new Error('incomplete
content-length'))) and ensure the underlying connection is torn down (e.g., call
this.uwsRes.close() or otherwise destroy the response) so the client receives a
connection error rather than a truncated response; update the _final()
implementation (referencing _final(), tryEnd(), this.contentLengthTotal,
this.finished, this.aborted, this.uwsRes.end(), this.uwsRes.close(), and
this.pendingFinalCallback) to perform these checks and call the
pendingFinalCallback/error path when incomplete instead of unconditionally
calling uwsRes.end().
---
Outside diff comments:
In `@src/http/core/response.ts`:
- Around line 197-213: The chunked branch of _write currently ignores
writeChunk()'s boolean return and calls callback() immediately, which drops
backpressure and allows unbounded growth of pendingChunks; change _write to
check the return of writeChunk(chunk, encoding) and only call callback() when
writeChunk returns true (or when backpressure clears), and if writeChunk returns
false enqueue the callback (e.g., pendingWriteCallbacks) to be invoked from
flushChunks() or the onWritable handler once pendingChunks have actually been
flushed; unify the drain handling so both the writeChunk-scheduled flush and the
onWritable handler resolve the same pending callback queue instead of racing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 037f7b72-5250-4cfc-93ac-2a623bc506ef
📒 Files selected for processing (1)
src/http/core/response.ts
VikramAditya33
force-pushed
the
fix/test-stream
branch
from
4ec422b to
2172209
Compare
…otocol violation & _write() bypasses chunk batching in chunked mode
VikramAditya33
force-pushed
the
fix/test-stream
branch
from
2172209 to
e512dce
Compare
1 participant
readable.pipe(res)When
res.setHeader('content-length', size)was set before piping a readable stream,_write()unconditionally useduWS.write()(chunked mode). Per RFC 7230 Section 3.3.2, Transfer-Encoding: chunked cannot coexist with Content-Length, causingHTTPParserErroron the client. Fixesreadable.pipe(res)with Content-Length header causes HTTP/1.1 protocol violation #104contentLengthTotalfield to track the declared sizesetHeader()/removeHeader()now detect and manage this field_write()routes totryEnd(totalSize)whencontentLengthTotalis definedwriteHead()skips writing the manual content-length header intryEndmode (avoids duplicate)_final()usesuwsRes.end()directly instead of send() for Content-Length mode_write()bypassed existing batching, causing excessivesyscalls_write()calledstreamChunk()per chunk, sending each to uWS individually. Forfs.createReadStream()with 4KB chunks, this resulted in ~128uWS.write()syscalls per 512KB instead of ~4 batched writes. Fixes_write()bypasses chunk batching in chunked mode #105routes _write()throughwriteChunk()which accumulates chunks inpendingChunks[]HIGH_WATERMARK(128KB) orFLUSH_INTERVAL(50ms)streamChunk()withtryEnd()unchangedSummary by CodeRabbit