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 <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Rich Trott <[email protected]>

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 c3fda6bCompare December 25, 2019 20:04

ronag commented Dec 25, 2019
View reviewed changes

lib/_stream_readable.js Outdated Show resolvedHide resolved

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

devnexen force-pushed the master branch from bdb1194 to 5289f80Compare 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 616c56eCompare January 1, 2020 12:10

BridgeAR reviewed Jan 1, 2020
View reviewed changes

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

ronag commented Jan 1, 2020
View reviewed changes

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

ronag commented Jan 1, 2020
View reviewed changes

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

ronag commented Jan 1, 2020
View reviewed changes

test/parallel/test-stream-writable-write-cb-twice.js Outdated Show resolvedHide 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

Uh oh!

stream: enable autoDestroy by default#30623

stream: enable autoDestroy by default #30623

Uh oh!

Conversation

ronag commented Nov 24, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ronag commented Nov 24, 2019

Uh oh!

mcollina commented Nov 24, 2019

Uh oh!

ronag commented Nov 24, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Nov 24, 2019

Uh oh!

ronag commented Nov 24, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollina commented Nov 24, 2019

Uh oh!

ronag commented Dec 1, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina commented Dec 1, 2019

Uh oh!

Trott commented Dec 2, 2019• edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 25, 2019

Uh oh!

BridgeAR commented Dec 25, 2019

Uh oh!

BridgeAR commented Dec 25, 2019

Uh oh!

ronag commented Dec 25, 2019

Uh oh!

BridgeAR commented Dec 25, 2019

Uh oh!

ronag commented Dec 25, 2019

Uh oh!

ronag commented Dec 25, 2019

Uh oh!

ronag commented Dec 25, 2019

Uh oh!

Uh oh!

nodejs-github-bot commented Dec 25, 2019

Uh oh!

ronag commented Dec 26, 2019

Uh oh!

nodejs-github-bot commented Dec 31, 2019

Uh oh!

ronag commented Jan 1, 2020

Uh oh!

nodejs-github-bot commented Jan 1, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented Jan 2, 2020• edited by BridgeAR Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Jan 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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