fix(http): server write first - #816
Conversation
📝 WalkthroughWalkthroughChangesTCP passthrough regression
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
|
@kettanaito Failing test for now. with |
![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
🤖 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 `@test/modules/http/regressions/http-non-http-socket-passthrough.test.ts`:
- Line 44: Remove the `.only` modifier from the `it` declaration for the “server
write first” test so the full regression test suite runs.
- Around line 51-60: Update the Promise in the socket response test to
accumulate data chunks rather than resolving on the first data event. Resolve
and destroy the socket only once the complete expected payload (PING or PONG, as
applicable) has been received, while retaining error rejection and the final
response assertion.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5106f9cb-e53e-445a-b7f6-a114efb1d867
📒 Files selected for processing (1)
test/modules/http/regressions/http-non-http-socket-passthrough.test.ts
| expect(response).toBe('PONG') | ||
| }) | ||
|
|
||
| it.only('server write first', async () => { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/modules/http/regressions/http-non-http-socket-passthrough.test.ts"
if [ -f "$file" ]; then
echo "== file exists =="
wc -l "$file"
echo "== relevant section =="
sed -n '1,90p' "$file" | cat -n
else
echo "file not found"
fd -a 'http-non-http-socket-passthrough.test.ts' .
fi
echo "== other it.only/it.se* in regression test files =="
rg -n "\b(it\.only|it\.serial|it\.sequential|it\.concurrent|it\.todo|describe\.only)\b" test/modules/http/regressions || trueRepository: mswjs/interceptors
Length of output: 2411
Remove it.only before merging.
This focuses Vitest on the new test and skips the other regression tests, which can let failures elsewhere go unnoticed.
Proposed fix
-it.only('server write first', async () => {
+it('server write first', async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it.only('server write first', async () => { | |
| it('server write first', async () => { |
🤖 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 `@test/modules/http/regressions/http-non-http-socket-passthrough.test.ts` at
line 44, Remove the `.only` modifier from the `it` declaration for the “server
write first” test so the full regression test suite runs.
| const response = await new Promise<string>((resolve, reject) => { | ||
| const socket = net.connect(server.port, server.hostname) | ||
| socket.on('data', (chunk) => { | ||
| resolve(chunk.toString()) | ||
| socket.destroy() | ||
| }) | ||
| socket.on('error', reject) | ||
| }) | ||
|
|
||
| expect(response).toBe('PING') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced test file and nearby code.
if [ -f "test/modules/http/regressions/http-non-http-socket-passthrough.test.ts" ]; then
echo "== file exists =="
wc -l test/modules/http/regressions/http-non-http-socket-passthrough.test.ts
echo
echo "== relevant test file =="
cat -n test/modules/http/regressions/http-non-http-socket-passthrough.test.ts
else
echo "Referenced file not found"
fd -i 'http-non-http-socket-passthrough.test.ts' .
fi
echo
echo "== related server/passthrough definitions =="
git ls-files | xargs rg -n "non-http|PING|passthrough|http-non-http-socket|connect\\(" test src 2>/dev/null | head -200 || trueRepository: mswjs/interceptors
Length of output: 23818
🌐 Web query:
Node.js net.Socket data event chunk partial receive documentation
💡 Result:
In Node.js, the net.Socket 'data' event does not guarantee the receipt of complete messages [1][2]. Because TCP is a stream-oriented protocol rather than a message-oriented one, there is no inherent concept of message boundaries [2][3]. A single logical message sent by a client can be split into multiple 'data' event chunks, or multiple messages can be merged into a single 'data' event chunk [2][4]. Key characteristics regarding 'data' event chunks include: 1. Arbitrary Chunk Size: The size of the 'data' event chunk is non-deterministic [5]. It depends on factors such as network conditions, kernel buffer availability, and the underlying stream's buffering behavior [5][2]. 2. Streaming Nature: When a net.Socket is in flowing mode, it emits 'data' events as soon as data arrives and is read from the kernel [1][5]. You should not assume that a 'data' event payload represents a full logical unit of data [4]. 3. Manual Boundary Handling: Because the network layer does not enforce boundaries, application-level code must implement its own mechanisms to delineate messages [2][3]. Common strategies include: - Length-prefixing: Sending the size of the message as a header before the actual message body [4]. - Delimiters: Using specific characters (e.g., newline \n or other markers) to signal the end of a message [3]. - Buffering: Accumulating received chunks into a buffer and parsing that buffer for complete messages whenever new data arrives [4][3]. If you need better control over how data is read or processed, you can consider using the 'readable' event and the readable.read method, which allow you to manually consume data from the stream's internal buffer instead of relying on the automatic 'data' event flow [6][7].
Citations:
- 1: https://nodejs.org/dist/latest/docs/api/net.html
- 2: https://stackoverflow.com/questions/40591455/nodejs-tcp-socket-sends-multiple-data-event
- 3: https://stackoverflow.com/questions/69167144/node-net-data-merge-when-multiple-write
- 4: https://stackoverflow.com/questions/5955498/node-js-tcp-server-incoming-buffer
- 5: https://stackoverflow.com/questions/40797337/what-determines-the-size-of-socket-data-event-in-nodejs
- 6: https://nodejs.org/api/stream.html
- 7: https://stackoverflow.com/questions/26174308/what-are-the-differences-between-readable-and-data-event-of-process-stdin-stream
Accumulate TCP chunks before asserting.
net.Socket data events are stream chunks, so resolving on the first chunk can truncate multi-chunk writes. Accumulate until the expected payload (PONG or PING) is complete before resolving.
🤖 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 `@test/modules/http/regressions/http-non-http-socket-passthrough.test.ts`
around lines 51 - 60, Update the Promise in the socket response test to
accumulate data chunks rather than resolving on the first data event. Resolve
and destroy the socket only once the complete expected payload (PING or PONG, as
applicable) has been received, while retaining error rejection and the final
response assertion.
|
@kettanaito I think I've found the culprit.
|
1 participant
No description provided.