Skip to content

Add sockets streams suite - #7180

Open
jasnell wants to merge 1 commit into
jasnell/streams-test-consolidation-8from
jasnell/streams-test-consolidation-9
Open

Add sockets streams suite#7180
jasnell wants to merge 1 commit into
jasnell/streams-test-consolidation-8from
jasnell/streams-test-consolidation-9

Conversation

@jasnell

@jasnell jasnell commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

More test consolidations.

@jasnell
jasnell requested review from a team as code owners August 28, 2026 23:01
Comment on lines +217 to +222
const socket = connect(greetAddress(env));
const reader = socket.readable.getReader();
await reader.read(); // take the greeting (or its first fragment)
await reader.cancel('done');
await socket.close();
ok(true);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The greet peer has already sent EOF before reader.cancel() runs, so this passes if cancellation is a no-op; socket.close() then supplies the observed closure. Keep the peer open, cancel an actually pending read, and observe the public closed promise.

Suggested change
const socket = connect(greetAddress(env));
const reader = socket.readable.getReader();
await reader.read(); // take the greeting (or its first fragment)
await reader.cancel('done');
await socket.close();
ok(true);
async test(ctrl, env) {
const socket = connect(echoAddress(env));
await socket.opened;
const reader = socket.readable.getReader();
const pendingRead = reader.read();
await reader.cancel('done');
await pendingRead;
await socket.close();
await socket.closed;
},

@ask-bonk

ask-bonk Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

I'm Bonk, and I've done a quick review of your PR.

Adds a dual-implementation socket-streams test suite with TCP echo sidecars.

  1. Medium cancelReadableSettlesSocket does not exercise cancellation of a live read. Posted one inline suggestion to correct it.

Verification could not complete: Bazel lacks libc++ headers in this runner.

github run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant