Fix #94 Return proper status code for large responses and #95 Support decompression#97
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
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/http/core/request.ts (1)
256-262: ⚡ Quick win
decompressionStream.write()ignores backpressure — unbounded memory growth under load.
Writable.write()returnsfalsewhen the stream's internal buffer is full and callers should stop writing until'drain'fires. By ignoring the return value, uWS data is pumped into the decompression stream unconditionally. For large or slowly-consumed compressed bodies this can cause the decompression stream's internal buffer to grow without bound.♻️ Suggested improvement
if (this.decompressionStream) { - this.decompressionStream.write(buffer); + if (!this.decompressionStream.write(buffer)) { + // Decompression stream buffer full — pause uWS until it drains + this.pause(); + this.decompressionStream.once('drain', () => this.resume()); + } if (isLast) { this.decompressionStream.end(); } return; }🤖 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/request.ts` around lines 256 - 262, The current block writing to this.decompressionStream ignores Writable.write() backpressure; change it to check the boolean return of this.decompressionStream.write(buffer) and, if it returns false, stop pushing more uWS data (queue the current chunk or mark stream as paused), attach a one-time 'drain' listener on this.decompressionStream to resume writing queued chunks and, if isLast was true, call this.decompressionStream.end() only after drain/resume; ensure you don't attach duplicate 'drain' handlers (use a flag or single listener) and correctly handle the isLast path so the stream is ended after buffered data is flushed.
🤖 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/request.ts`:
- Around line 943-946: The decompressor 'end' handler currently only sets
doneReadingData and emits 'received', which leaves streaming consumers hanging
because processDecompressedChunk only calls push(null) when isLast === true and
activateStreaming checks doneReadingData at activation time; update the
decompress.on('end') handler to also signal stream EOF by calling the same code
path that emits push(null) — e.g., mark doneReadingData = true, emit 'received',
and invoke the logic that pushes null to the readable stream (the same
termination behavior used when processDecompressedChunk is called with isLast
=== true) so streaming-mode consumers receive end even when decompression
finishes without a final chunk.
- Around line 948-967: In the decompress 'error' handler add setting
this.aborted = true (same way _onAbort does) so subsequent calls to
handleIncomingChunk will bail out before writing to the destroyed decompression
stream; specifically, inside the decompress.on('error', ...) block (where
this.flushing and this.abortError are set and responseWrapper/uwsRes handling
and this.destroy(...) occur) set this.aborted = true immediately after setting
this.abortError to prevent writes to the destroyed Transform in
handleIncomingChunk().
---
Nitpick comments:
In `@src/http/core/request.ts`:
- Around line 256-262: The current block writing to this.decompressionStream
ignores Writable.write() backpressure; change it to check the boolean return of
this.decompressionStream.write(buffer) and, if it returns false, stop pushing
more uWS data (queue the current chunk or mark stream as paused), attach a
one-time 'drain' listener on this.decompressionStream to resume writing queued
chunks and, if isLast was true, call this.decompressionStream.end() only after
drain/resume; ensure you don't attach duplicate 'drain' handlers (use a flag or
single listener) and correctly handle the isLast path so the stream is ended
after buffered data is flushed.
🪄 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: 14bf2ee4-c9f4-4a7a-9f43-7769db5c6b3a
📒 Files selected for processing (1)
src/http/core/request.ts
Content-Encoding: gzip/deflate/brare now automatically decompressed.maxBodySize, it now returns proper HTTP 413 Payload Too Large response.Fix #94
Fix #95
Summary by CodeRabbit
New Features
Bug Fixes