stream: avoid unhandled exception when destroy w/ callback.

[ronag]

Providing a callback to destroy means that the error should be handled through that callback and thus an unhandled exception error should not be necessary.

Some minor preparation towards possibly making the destroy(err, cb) signature public.

With "better" ergonomics I'm hoping users that usually skip adding an 'error' listeners before destroy() no longer end up with unhandled exceptions in edge cases.

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

[mscdex]

Perhaps here and elsewhere in this function we should instead check typeof cb === 'function'?

[ronag]

We don't do that anywhere else in this file. If you'd prefer it like that, I would prefer to handle that in a separate PR and change it everywhere in this file.

[ronag]

@lpinca: Are you ok with this change? The error is not swallowed per se.

[lpinca]

Is there a particular reason? Adding 'error' events is user responsibility and it is has been a Node.js feature from the very early days. I don't think we should make it optional if a callback is passed to stream.destroy(). We add no default listener if a callback is passed to writable.write().

[lpinca]

@nodejs/streams

[ronag]

For me this is about the unhandled exception behavior

I don't think we should make it optional if a callback is passed to stream.destroy().

We emit an unhandled exception error and close the program because the exception is supposedly unhandled. If the user provides a callback and do handle it, this behavior is in my opinion both unintuitive and unnecessary.

We add no default listener if a callback is passed to writable.write().

I think that might be worth into looking into / consider updating those and others as well. I was hoping to propose that in a separate semver major PR if there is consensus on this one.

[ronag]

For context I'm just probing a bit in regards to how much of normalized destroy would make sense to move into core streams and how much makes sense to be an outside util.

[mcollina]

We are using that callback internally at least once. Can you please verify where we are using it right now and if this would break them?

I think this is semver-major.

Docs are missing.

[ronag]

We are using that callback internally at least once. Can you please verify where we are using it right now and if this would break them?

• fs.ReadStream.close(cb)
• onWriteComplete

onWriteComplete unfortunately uses it which might change the behavior more than intended. Probably safe but we might as well wait for #29179 which would fix this. So this PR is probably blocked by that.

Given the limited use we could also fix so that the cb is invoked asynchronously like the rest.

Docs are missing.

This is not a public API (yet).

[BridgeAR]

This needs a rebase.

[ronag]

Blocked by #29179

[ronag]

Been some related progress on other PR's. Closing this for now and re-opening later.