stream: finished on closed OutgoingMessage

[ronag]

finished should invoke callback on closed OutgoingMessage the
same way as for regular streams.

Fixes: #34274

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

[ronag]

@nodejs/streams

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mcollina]

lgtm

[rickyes]

finished should invoke callback on closed OutgoingMessage the
same way as for regular streams.

Fixes: #34274

The fix should be #34301

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[ronag]

@Trott Help. I can't get node-test-commit-custom-suites-freestyle (test-worker) to pass (test.parallel/test-bootstrap-modules).

[Trott]

@Trott Help. I can't get node-test-commit-custom-suites-freestyle (test-worker) to pass (test.parallel/test-bootstrap-modules).

Does it fail for you locally with tools/test.py --worker test/parallel/test-bootstrap-modules.js?

[richardlau]

@Trott Help. I can't get node-test-commit-custom-suites-freestyle (test-worker) to pass (test.parallel/test-bootstrap-modules).

Does it fail for you locally with tools/test.py --worker test/parallel/test-bootstrap-modules.js?

👍

This test is designed to highlight when we cause extra modules to be loaded during the bootstrap and make sure that we're are doing that conciously. In this case, we're now dragging in https://github.com/nodejs/node/blob/master/lib/internal/http.js during bootstrap with workers because workers uses streams. This is in turn also dragging in the internal performance binding:

node/lib/internal/http.js

Line 8 in 66810a0

const { PerformanceEntry, notify } = internalBinding('performance');

If loading those extra modules now when we bootstrap worker threads is acceptable, add them to the appropriate parts of the test (there's a conditional section for workers). Otherwise consider lazy loading the extra module(s) so they are only loaded if needed.

[ronag]

I'm unsure how to fix this then... where do we put kClosed which is only used for http?

[nodejs-github-bot]

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

[ronag]

@nodejs/http

[nodejs-github-bot]

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

[mcollina]

lgtm

[ronag]

Landed in a55b77d

[vweevers]

Could be an argument for reconsidering #33990

[ronag]

Could be an argument for reconsidering #33990

I don't think internal difficulties should motivate public APIs?

[vweevers]

OutgoingMessage has ._closed, fs streams have .closed, might be worth consolidating. Without it, can finished() detect already-closed userland streams?

[ronag]

Without it, can finished() detect already-closed userland streams?

Yes? Using the internal property _state.closed? Depends on what you mean by "userland streams". You mean streams that are streamlike but not actually streams?

OutgoingMessage has ._closed

internal property

fs streams have .closed

public property

I'm not against it (I opened that PR after all) but I'm not for it either. I don't think this specific case motivates it.