fs: do not emit open and read if stream destroyed

[dexterleng]

WIP, will write tests ASAP.

References: #23133

Addresses this:

@mcollina I don’t understand… I’m not talking about changing emit() to be a noop function or so, I’m talking about the individual call sites in our own net, tls, fs, etc. code?

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[Trott]

@nodejs/fs @mcollina

[mcollina]

Thanks for getting started with this! I think it's almost there.

[mcollina]

we are swallowing the original error here.

[mcollina]

LGTM

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/1​89​94
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1​65​3

[rvagg]

fine by me
🔥🔥 alternatively we could just burn streams to the ground and start again, after this security release I'm in the mood 🔥🔥

[mcollina]

This will cause error to be emitted after close. I think we should swallow this error.

[dexterleng]

I removed it and the error still got emitted. The error event does not seem to come from lib/internal/fs/streams.js... 🤔

Path: parallel/test-fs-stream-no-events-after-destroy
assert.js:128
  throw err;
  ^

AssertionError [ERR_ASSERTION]: function should not have been called at /Users/hackintosh/Documents/code/node/test/parallel/test-fs-stream-no-events-after-destroy.js:15
    at ReadStream.mustNotCall (/Users/hackintosh/Documents/code/node/test/common/index.js:404:12)
    at ReadStream.emit (events.js:189:13)
    at emitErrorNT (internal/streams/destroy.js:82:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:50:3)
    at internalTickCallback (internal/process/next_tick.js:72:19)
    at process._tickCallback (internal/process/next_tick.js:47:5)
    at Function.Module.runMain (internal/modules/cjs/loader.js:778:11)
    at startup (internal/bootstrap/node.js:300:19)
    at bootstrapNodeJSCore (internal/bootstrap/node.js:826:3)

[mcollina]

CI: https://ci.nodejs.org/job/node-test-pull-request/19715/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1695/

[mcollina]

CI: https://ci.nodejs.org/job/node-test-commit/24545/

[mcollina]

There are about 2 tests failing:

test.parallel/test-fs-stream-no-events-after-destroy
test.parallel/test-fs-stream-double-close

On various environments. If they pass locally, it means they are flaky on CI: you can run a stress test locally with python tools/test.py --repeat 1024 test/parallel/test-fs-stream-no-events-after-destroy.js as an example.

While you are working on it, could you also rebase on top of master?

[mcollina]

Using "request changes" so that it does not get landed until tests are fixed.

[antsmartian]

Ping @dexterleng 🔉

[dexterleng]

Im not sure how to prevent errors from being swallowed.
I've done this:

node/lib/internal/fs/streams.js

Line 137 in 930c8d4

// swallow the error to prevent error from emitting after close.

If someone else knows how to fix this please help out.

[mcollina]

I don't understand what is the problem you are facing as the change looks fine to me. It seems some tests would require some changes as a result

[antsmartian]

ping @dexterleng . Is this something you are planning to work on?

[dexterleng]

@antsmartian I am no longer working on it.

[jasnell]

Closing due to lack of continued activity