net/streams: don't return the stream object from onStreamRead#34375

robey · 2020-07-15T03:54:15Z

CallJSOnreadMethod expects the return value to be undefined or
a new buffer, so make sure to return nothing, even when an error
causes us to destroy the stream.

Fixes: #34346

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
- (except test-benchmark-napi, which has been broken since I cloned)
commit message follows commit guidelines

lpinca · 2020-07-15T10:12:18Z

Is it possible to add a regression test?

robey · 2020-07-15T16:25:50Z

i think you would need to have some integration tests, sadly. it doesn't seem possible for a connection to be "reset" between two sockets on the same machine. (my understanding is that ECONNRESET is a response to unexpectedly receiving a TCP RST.)

another way you could do it is to fake up the ECONNRESET by mucking around in the internals somehow, but i'm not sure how you can trick libuv into doing that. would be cool tho!

lpinca · 2020-07-15T17:14:47Z

I think net: is a better subsystem in commit title.

ronag

There is another instance of this here https://github.com/nodejs/node/pull/34375/files#diff-704db381d3305bdcd2c6b8b3468acfb7R230

robey · 2020-07-18T00:22:47Z

good eye! i went ahead and fixed that one too, while i was in here.

mcollina · 2020-07-18T06:58:01Z

Can you please add a unit test?

robey · 2020-07-18T18:17:19Z

as i said above, if you can think of a way to do it without having a cross-process integration test, i'm happy to try it.

mcollina · 2020-07-18T20:57:45Z

would it be possible to use child processes? We have a few tests that use them for similar purposes.

lib/internal/stream_base_commons.js

addaleax

I would probably also update the crash site in stream_base.cc to check for next_buf_v->IsArrayBufferView() instead of !next_buf_v->IsUndefined() (although that can be done as a separate fix)

robey · 2020-07-19T00:56:37Z

i figured out some sample code that causes the crash (see bug), so i added that as a unit test. :)

robey · 2020-07-19T01:21:48Z

I would probably also update the crash site in stream_base.cc to check for next_buf_v->IsArrayBufferView() instead of !next_buf_v->IsUndefined() (although that can be done as a separate fix)

sure, added that too. :)

mcollina

lgtm

addaleax

Still LGTM :)

nodejs-github-bot · 2020-07-20T18:27:56Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32426/

nodejs-github-bot · 2020-07-21T19:18:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32451/

jasnell · 2020-07-23T14:33:16Z

Looks like there are a number of possibly relevant failures in CI

robey · 2020-07-23T18:15:37Z

Looks like there are a number of possibly relevant failures in CI

i assumed these were all spurious since "make test" passes cleanly (except the one that always fails). which test is failing for you and how can i repro it?

lpinca · 2020-07-23T18:53:49Z

@robey here are a few examples:

node-test-commit-freebsd

21:31:45 not ok 1645 parallel/test-net-onread-static-buffer 21:31:45 --- 21:31:45 duration_ms: 0.441 21:31:45 severity: fail 21:31:45 exitcode: 1 21:31:45 stack: |- 21:31:45 Mismatched <anonymous> function calls. Expected exactly 1, actual 0. 21:31:45 at Proxy.mustCall (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/common/index.js:331:10) 21:31:45 at Server.<anonymous> (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd11-x64/test/parallel/test-net-onread-static-buffer.js:203:29) 21:31:45 at Object.onceWrapper (events.js:420:28) 21:31:45 at Server.emit (events.js:314:20) 21:31:45 at emitListeningNT (net.js:1322:10) 21:31:45 at processTicksAndRejections (internal/process/task_queues.js:79:21)

node-test-commit-smartos

21:31:32 not ok 1470 parallel/test-net-onread-static-buffer 21:31:32 --- 21:31:32 duration_ms: 0.536 21:31:32 severity: fail 21:31:32 exitcode: 1 21:31:32 stack: |- 21:31:32 Mismatched <anonymous> function calls. Expected exactly 1, actual 0. 21:31:32 at Proxy.mustCall (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/test/common/index.js:331:10) 21:31:32 at Server.<anonymous> (/home/iojs/build/workspace/node-test-commit-smartos/nodes/smartos18-64/test/parallel/test-net-onread-static-buffer.js:203:29) 21:31:32 at Object.onceWrapper (events.js:420:28) 21:31:32 at Server.emit (events.js:314:20) 21:31:32 at emitListeningNT (net.js:1322:10) 21:31:32 at processTicksAndRejections (internal/process/task_queues.js:79:21)

node-test-commit-windows-fanned

21:26:37 not ok 409 parallel/test-net-onread-static-buffer 21:26:37 --- 21:26:37 duration_ms: 0.633 21:26:37 severity: fail 21:26:37 exitcode: 134 21:26:37 stack: |- 21:26:37 Administrator: Administrator: Windows PowerShell[8864]: C:\workspace\node-compile-windows\node\src\stream_base.cc:534: Assertion `(buf.base) == (buffer_.base)' failed. 21:26:37 1: 00007FF64E5CA33F v8::internal::Isolate::ArchiveSpacePerThread+4127 21:26:37 2: 00007FF64E56AC06 v8::internal::wasm::WasmCode::safepoint_table_offset+65046 21:26:37 3: 00007FF64E56AF81 v8::internal::wasm::WasmCode::safepoint_table_offset+65937 21:26:37 4: 00007FF64E4917D1 v8::internal::Isolate::isolate_root_bias+15889 21:26:37 5: 00007FF64E48C639 v8::internal::MicrotaskQueue::microtasks_policy+1257 21:26:37 6: 00007FF64E60939F uv_tty_set_vterm_state+8847 21:26:37 7: 00007FF64E61EE8C uv_loop_init+844 21:26:37 8: 00007FF64E61F19A uv_run+202 21:26:37 9: 00007FF64E523D25 v8::internal::OrderedHashTable<v8::internal::OrderedHashSet,1>::NumberOfBucketsOffset+10277 21:26:37 10: 00007FF64E598AC6 node::Start+262 21:26:37 11: 00007FF64E3F785C RC4_options+343900 21:26:37 12: 00007FF64F38B00C v8::internal::compiler::RepresentationChanger::Uint32OverflowOperatorFor+156156 21:26:37 13: 00007FFDFF637BD4 BaseThreadInitThunk+20 21:26:37 14: 00007FFE00ACCE51 RtlUserThreadStart+33

ronag · 2020-07-23T18:55:38Z

src/stream_base.cc

Might this be what's causing problems?

makes sense... ok, i reverted that part.

CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346

robey · 2020-08-07T19:12:00Z

it doesn't seem like there's any interest in this patch, so i'll close it next week. we can run a fork of node internally. sorry it didn't work out.

nodejs-github-bot · 2020-08-07T19:17:51Z

CI: https://ci.nodejs.org/job/node-test-pull-request/32675/

lpinca · 2020-08-07T19:54:01Z

CI is green. I think this can land.

addaleax · 2020-08-08T20:03:35Z

Landed in c0be31f, and sorry for the delay!

CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346 PR-URL: #34375 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

lpinca approved these changes Jul 15, 2020
View reviewed changes

ronag requested changes Jul 15, 2020
View reviewed changes

robey requested a review from a teamJuly 18, 2020 00:22

robey changed the title ~~streams: don't return the stream object from onStreamRead~~net/streams: don't return the stream object from onStreamReadJul 18, 2020

mcollina reviewed Jul 18, 2020
View reviewed changes

lib/internal/stream_base_commons.js Outdated Show resolvedHide resolved
lib/internal/stream_base_commons.js Outdated Show resolvedHide resolved

addaleax approved these changes Jul 18, 2020
View reviewed changes

mcollina approved these changes Jul 19, 2020
View reviewed changes

robey requested a review from ronag July 20, 2020 18:21

ronag approved these changes Jul 20, 2020
View reviewed changes

addaleax approved these changes Jul 20, 2020
View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 20, 2020

jasnell added net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 23, 2020

ronag reviewed Jul 23, 2020
View reviewed changes

streams: don't return the stream object from onStreamRead
0865a22
CallJSOnreadMethod expects the return value to be undefined or a new buffer, so make sure to return nothing, even when an error causes us to destroy the stream. Fixes: #34346

lpinca added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 7, 2020

addaleax closed this Aug 8, 2020

codebytere mentioned this pull request Aug 10, 2020
v14.8.0 proposal #34704
Merged

codebytere mentioned this pull request Sep 28, 2020
v12.19.0 proposal #35401
Merged

github-actionsbot mentioned this pull request Apr 24, 2024
Adding VersionStream for nodejs-22 wolfi-dev/os#17646
Merged

Uh oh!

net/streams: don't return the stream object from onStreamRead#34375

net/streams: don't return the stream object from onStreamRead #34375

Uh oh!

Conversation

robey commented Jul 15, 2020

Checklist

Uh oh!

lpinca commented Jul 15, 2020

Uh oh!

robey commented Jul 15, 2020

Uh oh!

lpinca commented Jul 15, 2020

Uh oh!

ronag left a comment

Choose a reason for hiding this comment

Uh oh!

robey commented Jul 18, 2020

Uh oh!

mcollina commented Jul 18, 2020

Uh oh!

robey commented Jul 18, 2020

Uh oh!

mcollina commented Jul 18, 2020

Uh oh!

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

robey commented Jul 19, 2020

Uh oh!

robey commented Jul 19, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 20, 2020

Uh oh!

nodejs-github-bot commented Jul 21, 2020

Uh oh!

jasnell commented Jul 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robey commented Jul 23, 2020

Uh oh!

lpinca commented Jul 23, 2020

Uh oh!

ronagJul 23, 2020

Choose a reason for hiding this comment

Uh oh!

robeyJul 24, 2020

Choose a reason for hiding this comment

Uh oh!

robey commented Aug 7, 2020

Uh oh!

nodejs-github-bot commented Aug 7, 2020

Uh oh!

lpinca commented Aug 7, 2020

Uh oh!

addaleax commented Aug 8, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

jasnell commented Jul 23, 2020•
edited
Loading