stream: enable autoDestroy by default #30623

ronag · 2019-11-24T12:18:37Z

Enable autoDestroy by default.

semver-major

Refs: #30621

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 · 2019-11-24T12:40:26Z

This will need CITGM

mcollina · 2019-11-24T12:54:39Z

Can you open up a tracking issues with all the node core implementations that would need to be updated to autoDestroy: true?

ronag · 2019-11-24T12:56:52Z

Can you open up a tracking issues with all the node core implementations that would need to be updated to autoDestroy: true?

I think you mean autoDestroy: false? I've already updated them in this PR. Or am I missing/misunderstanding something?

mcollina · 2019-11-24T12:58:15Z

I think we should migrate all of those you have set to false in this PR to true before node v14 is cut.

ronag · 2019-11-24T13:02:37Z

I think we should migrate all of those you have set to false in this PR to true before node v14 is cut.

#30625

Though realistically I don't think we will be able to do this for internal/http2/core.js.

Just to clarify. Are you proposing we do not merge this PR until we've migrated?

mcollina · 2019-11-24T14:02:20Z

I’m fine with landing this before that happens.

ronag · 2019-12-01T18:51:18Z

@Trott: I think this needs a CITGM?

@mcollina: Do you think you can approve this PR or is there something further that needs to be addressed?

mcollina

lgtm

mcollina · 2019-12-01T21:50:47Z

cc @nodejs/tsc this needs a review.

Trott · 2019-12-02T05:35:35Z

~~CITGM: https://ci.nodejs.org/view/All/job/citgm-smoker/2103/~~
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2157/

nodejs-github-bot · 2019-12-25T09:32:10Z

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

BridgeAR · 2019-12-25T09:33:00Z

@nodejs/tsc this has three LGs but it needs one more LG from the TSC. PTAL

BridgeAR · 2019-12-25T11:22:42Z

Seems like this has failing test cases.

ronag · 2019-12-25T19:14:21Z

Seems like this has failing test cases.

Not sure how to resolve those. I'm running on OSX and I can't reproduce the OSX failures. Are we sure the CI is correct?

BridgeAR · 2019-12-25T19:29:45Z

@ronag these two tests fail (even on OSX):

test/parallel/test-stream-catch-rejections.js
test/parallel/test-stream-writable-write-cb-twice.js

You can click on the failed results here and you should get an overview of Jenkins with the specific Job that was run. E.g., https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1011/30941/.

You can click on the specific part of the job and see e.g., the failure output:
https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1011/30941/testReport/junit/(root)/test/parallel_test_stream_catch_rejections/
https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1011/30941/testReport/junit/(root)/test/parallel_test_stream_writable_write_cb_twice/

ronag · 2019-12-25T19:38:45Z

@BridgeAR: Something is wrong here test-stream-catch-rejections doesn't even exist. Did this CI run against a different branch/PR?

ronag · 2019-12-25T19:41:08Z

Ah, it does a merge against master. I'll rebase this and re-run tests.

ronag · 2019-12-25T20:04:24Z

@BridgeAR: another CI please

lib/_stream_readable.js

nodejs-github-bot · 2019-12-25T20:09:51Z

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

ronag · 2019-12-26T19:26:13Z

rebased

nodejs-github-bot · 2019-12-31T05:30:40Z

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

ronag · 2020-01-01T12:10:31Z

rebased to fix conflicts

nodejs-github-bot · 2020-01-01T12:27:32Z

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

test/parallel/test-stream-writable-write-cb-twice.js

nodejs-github-bot · 2020-01-02T19:21:21Z

CI: https://ci.nodejs.org/job/node-test-pull-request/28119/ ✔️

PR-URL: #30623 Refs: #30621 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Rich Trott <rtrott@gmail.com>

BridgeAR · 2020-01-03T00:50:34Z

Landed in 4bec6d1 🎉

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Nov 24, 2019

lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 24, 2019

lpinca approved these changes Nov 24, 2019

View reviewed changes

ronag mentioned this pull request Nov 24, 2019

stream: autoDestroy: true #30625

Closed

6 tasks

mcollina approved these changes Dec 1, 2019

View reviewed changes

BridgeAR approved these changes Dec 25, 2019

View reviewed changes

ronag force-pushed the stream-auto-destroy branch 2 times, most recently from 052da2a to c3fda6b Compare December 25, 2019 20:04

ronag commented Dec 25, 2019

View reviewed changes

lib/_stream_readable.js Show resolved Hide resolved

ronag force-pushed the stream-auto-destroy branch from 6327773 to 58424ee Compare December 26, 2019 19:23

devnexen force-pushed the master branch from bdb1194 to 5289f80 Compare December 26, 2019 19:44

Trott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs. labels Dec 27, 2019

ronag mentioned this pull request Dec 29, 2019

[x] http: client destroy stream #29192

Closed

4 tasks

ronag added 6 commits January 1, 2020 13:09

stream: enable autoDestroy by default

df2d395

fixup: linting

3e188d7

fixup: tests

d9ff59b

fixup: test

28cf38d

fixup: docs

1899ada

fixup: tests + Readable capture

616c56e

ronag force-pushed the stream-auto-destroy branch from 58424ee to 616c56e Compare January 1, 2020 12:10

BridgeAR reviewed Jan 1, 2020

View reviewed changes

test/parallel/test-stream-writable-write-cb-twice.js Outdated Show resolved Hide resolved

ronag commented Jan 1, 2020

View reviewed changes

test/parallel/test-stream-writable-write-cb-twice.js Outdated Show resolved Hide resolved

ronag commented Jan 1, 2020

View reviewed changes

test/parallel/test-stream-writable-write-cb-twice.js Outdated Show resolved Hide resolved

ronag commented Jan 1, 2020

View reviewed changes

test/parallel/test-stream-writable-write-cb-twice.js Outdated Show resolved Hide resolved

ronag added 2 commits January 1, 2020 14:49

Apply suggestions from code review

ccdfafe

fixup

1bb3889

This comment has been minimized.

Sign in to view

BridgeAR closed this Jan 3, 2020

davedoesdev mentioned this pull request Jan 18, 2020

Deprecating autoDestroy: false and autoClose: false #30621

Closed

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

kanongil mentioned this pull request Aug 26, 2020

fix: do not auto destroy request stream hapijs/shot#132

Merged

phuze mentioned this pull request May 10, 2021

SFTP error upload no such file after update VS code 1.56.0 liximomo/vscode-sftp#919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: enable autoDestroy by default #30623

stream: enable autoDestroy by default #30623

ronag commented Nov 24, 2019 •

edited

Loading

ronag commented Nov 24, 2019

mcollina commented Nov 24, 2019

ronag commented Nov 24, 2019 •

edited

Loading

mcollina commented Nov 24, 2019

ronag commented Nov 24, 2019 •

edited

Loading

mcollina commented Nov 24, 2019

ronag commented Dec 1, 2019

mcollina left a comment

mcollina commented Dec 1, 2019

Trott commented Dec 2, 2019 •

edited by BridgeAR

Loading

nodejs-github-bot commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

ronag commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

ronag commented Dec 25, 2019

ronag commented Dec 25, 2019

ronag commented Dec 25, 2019

nodejs-github-bot commented Dec 25, 2019

ronag commented Dec 26, 2019

nodejs-github-bot commented Dec 31, 2019

ronag commented Jan 1, 2020

nodejs-github-bot commented Jan 1, 2020

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 2, 2020 •

edited by BridgeAR

Loading

BridgeAR commented Jan 3, 2020

stream: enable autoDestroy by default #30623

stream: enable autoDestroy by default #30623

Conversation

ronag commented Nov 24, 2019 • edited Loading

Checklist

ronag commented Nov 24, 2019

mcollina commented Nov 24, 2019

ronag commented Nov 24, 2019 • edited Loading

mcollina commented Nov 24, 2019

ronag commented Nov 24, 2019 • edited Loading

mcollina commented Nov 24, 2019

ronag commented Dec 1, 2019

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented Dec 1, 2019

Trott commented Dec 2, 2019 • edited by BridgeAR Loading

nodejs-github-bot commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

ronag commented Dec 25, 2019

BridgeAR commented Dec 25, 2019

ronag commented Dec 25, 2019

ronag commented Dec 25, 2019

ronag commented Dec 25, 2019

nodejs-github-bot commented Dec 25, 2019

ronag commented Dec 26, 2019

nodejs-github-bot commented Dec 31, 2019

ronag commented Jan 1, 2020

nodejs-github-bot commented Jan 1, 2020

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 2, 2020 • edited by BridgeAR Loading

BridgeAR commented Jan 3, 2020

ronag commented Nov 24, 2019 •

edited

Loading

ronag commented Nov 24, 2019 •

edited

Loading

ronag commented Nov 24, 2019 •

edited

Loading

Trott commented Dec 2, 2019 •

edited by BridgeAR

Loading

nodejs-github-bot commented Jan 2, 2020 •

edited by BridgeAR

Loading