stdio: make stdout and stderr emit 'close' on destroy by mcollina · Pull Request #26691 · nodejs/node

mcollina · 2019-03-15T19:26:28Z

The problem in #26550 was caused by net.Socket having set emitClose: false because it was emitted its own event in _destroy(), and stdio.js was overriding _destroy without emitting 'close'.

Fix: #26550

Checklist

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

Fix: nodejs#26550

nodejs-github-bot · 2019-03-15T19:26:30Z

@mcollina build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2929/pipeline

mcollina · 2019-03-15T19:30:13Z

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

BridgeAR · 2019-03-15T21:48:14Z

+ if (!this._writableState.emitClose) {
+ process.nextTick(() => {
+ this.emit('close');
+ });


How come the nextTick is required in this case?

The previous cb() could emitting an error via nextTick. Close should be emitted afterwards.

Are you ok/can this PR land, or would you like to see some different resolution for this?

mcollina · 2019-03-16T11:01:39Z

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

addaleax · 2019-03-16T12:03:12Z

+ // SyncWriteStream does not use the stream
+ // destroy mechanism for some legacy reason.
+ // TODO(mcollina): remove when
+ // https://github.com/nodejs/node/pull/26690 lands.


What exactly are you intending to remove? The dummyDestroy() function? It’s intentional that stdio streams, SyncWriteStream or not, cannot “really” be closed (so that file descriptors 0, 1, 2 don’t accidentally get closed).

The if check underneath. See the linked PR. Basically we can have a callback or not for this function.

#26690 was merged so this needs to go now?

We can! However that was a semver-major commit and this is not. I would prefer to issue a follow-up PR that will not be backported. This can (and should) be backported.

mcollina · 2019-03-17T10:04:33Z

@nodejs/build can you take a look at https://ci.nodejs.org/job/node-test-commit-linux/26139/nodes=ubuntu1604-64/console? there seems a problem with that machine, if failed a couple of times.

mcollina · 2019-03-17T10:05:55Z

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

rvagg · 2019-03-17T12:09:54Z

I've updated, cleaned and rebooted that ubuntu machine, will see if that helps

Trott · 2019-03-17T20:45:25Z

there seems a problem with that machine, if failed a couple of times.

A root-owned directory was left around after some troubleshooting (generating a core file for another issue), and git operations were failing because they couldn't clean up the directory. I removed the directory several hours ago (when I noticed the problem because it failed in node-daily-master) and it should be back to working.

Resume Build CI: https://ci.nodejs.org/job/node-test-pull-request/21615/ ✅

mcollina · 2019-03-20T18:57:08Z

I will look to land this next Monday if there are no further objections. I'd also open a new PR to remove that if check at the same time.

mcollina · 2019-03-25T11:29:59Z

Landed in bdea725

Fix: #26550 PR-URL: #26691 Fixes: https://github.com/falseFixes: #26550 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs#26691 introduced an if to protect against SyncWriteStream not using the default .destroy() mechanism. This change removes that as SyncWriteStream now use standard .destroy(). See: nodejs#26691

lpinca · 2019-03-25T13:35:40Z

Commit landed with messed up metadata, I guess it's too late to fix it.

mcollina · 2019-03-25T16:54:36Z

Ouch, I didn't notice. I relied too blindly on git node land.

joyeecheung · 2019-03-25T19:47:09Z

We should probably add some stricter checks in https://github.com/nodejs/core-validate-commit for these cases

Fix: nodejs#26550 PR-URL: nodejs#26691 Fixes: https://github.com/falseFixes: nodejs#26550 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Fix: #26550 PR-URL: #26691 Fixes: https://github.com/falseFixes: #26550 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

#26691 introduced an if to protect against SyncWriteStream not using the default .destroy() mechanism. This change removes that as SyncWriteStream now use standard .destroy(). See: #26691 PR-URL: #26902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

richardlau · 2019-03-31T01:46:03Z

We should probably add some stricter checks in https://github.com/nodejs/core-validate-commit for these cases

nodejs/core-validate-commit#54

#26691 introduced an if to protect against SyncWriteStream not using the default .destroy() mechanism. This change removes that as SyncWriteStream now use standard .destroy(). See: #26691 PR-URL: #26902 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

BethGriggs · 2019-07-09T15:36:26Z

@mcollina should this still land on v10.x?

mcollina · 2019-07-09T16:26:24Z

Yes please

Fix: #26550 PR-URL: #26691 Fixes: https://github.com/falseFixes: #26550 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

stdio: make stdout and stderr emit 'close' on destroy
6e8c1e6
Fix: nodejs#26550

mcollina requested review from addaleax and gireeshpunathil March 15, 2019 19:26

nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Mar 15, 2019

mcollina requested a review from bnoordhuis March 15, 2019 19:27

mcollina mentioned this pull request Mar 15, 2019
Stream.pipeline doesn't report failures if process.stdout is in the pipeline. #26550
Closed

mcollina added lts-watch-v10.x labels Mar 15, 2019

BridgeAR reviewed Mar 15, 2019
View reviewed changes

jasnell approved these changes Mar 16, 2019
View reviewed changes

addaleax reviewed Mar 16, 2019
View reviewed changes

addaleax approved these changes Mar 16, 2019
View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 18, 2019

mcollina closed this Mar 25, 2019

mcollina deleted the fix-26550 branch March 25, 2019 11:31

mcollina mentioned this pull request Mar 25, 2019
process: remove protection for SyncWriteStream destroy in stdio #26902
Closed
2 tasks

targos mentioned this pull request Mar 27, 2019
v11.13.0 release proposal #26949
Merged

richardlau mentioned this pull request Mar 31, 2019
Stricter URL checks nodejs/core-validate-commit#54
Merged

BethGriggs added land-on-v10.x and removed lts-watch-v10.x labels Jul 9, 2019

BethGriggs mentioned this pull request Jul 17, 2019
v10.16.1 proposal #28731
Merged

Uh oh!

Uh oh!

Conversation

mcollina commented Mar 15, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Mar 15, 2019

Uh oh!

mcollina commented Mar 15, 2019

Uh oh!

BridgeARMar 15, 2019

Choose a reason for hiding this comment

Uh oh!

mcollinaMar 15, 2019

Choose a reason for hiding this comment

Uh oh!

mcollinaMar 17, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 16, 2019

Uh oh!

addaleaxMar 16, 2019

Choose a reason for hiding this comment

Uh oh!

mcollinaMar 16, 2019

Choose a reason for hiding this comment

Uh oh!

bnoordhuisMar 20, 2019

Choose a reason for hiding this comment

Uh oh!

mcollinaMar 20, 2019

Choose a reason for hiding this comment

Uh oh!

mcollina commented Mar 17, 2019

Uh oh!

mcollina commented Mar 17, 2019

Uh oh!

rvagg commented Mar 17, 2019

Uh oh!

Trott commented Mar 17, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Mar 20, 2019

Uh oh!

mcollina commented Mar 25, 2019

Uh oh!

lpinca commented Mar 25, 2019

Uh oh!

mcollina commented Mar 25, 2019

Uh oh!

joyeecheung commented Mar 25, 2019

Uh oh!

richardlau commented Mar 31, 2019

Uh oh!

BethGriggs commented Jul 9, 2019

Uh oh!

mcollina commented Jul 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

mcollina commented Mar 15, 2019•
edited
Loading

Trott commented Mar 17, 2019•
edited
Loading