stream: avoid destroying http1 objects by ronag · Pull Request #32197 · nodejs/node

ronag · 2020-03-11T12:44:44Z

http1 objects are coupled with their corresponding
res/req and cannot be treated independently as
normal streams. Add a special exception for this
in the pipeline cleanup.

Fixes: #32184

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 · 2020-03-11T12:45:38Z

I'm sorry about 8a2b62e causing these troubles. I've tried to resolve these follow up issues as quickly as possible.

nodejs-github-bot · 2020-03-11T13:00:05Z

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

mcollina · 2020-03-11T13:10:08Z

@ronag are we sure we should not revert all of those fixes instead and redo a PR with all of them?

mcollina

LGTM

ronag · 2020-03-11T13:19:57Z

@ronag are we sure we should not revert all of those fixes instead and redo a PR with all of them?

I would prefer not. Seems more difficult than fixing it.

I see your point though. If you would prefer revert I would be ok with that.

mcollina · 2020-03-11T13:23:00Z

Let's see if something else pop up.

nodejs-github-bot · 2020-03-11T13:36:19Z

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

MylesBorins · 2020-03-11T15:44:08Z

can we fast track this? I'd like to see it land in the upcoming 13.11.0 release

@ronag looks like this needs a rebase

http1 objects are coupled with their corresponding res/req and cannot be treated independently as normal streams. Add a special exception for this in the pipeline cleanup. Fixes: nodejs#32184

ronag · 2020-03-11T15:51:38Z

rebased @MylesBorins

nodejs-github-bot · 2020-03-11T15:52:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/29733/
retry linter: https://ci.nodejs.org/job/node-test-linter/33328/

mcollina · 2020-03-11T16:57:53Z

+1 to fast track

MylesBorins · 2020-03-11T17:23:48Z

@ronag FWIW this patch is not going to land cleanly on v13.x. You might want to get started on a backport now if you have time. very much trying to get this out in today's release.

MylesBorins · 2020-03-11T19:04:06Z

landed in f8f2089

http1 objects are coupled with their corresponding res/req and cannot be treated independently as normal streams. Add a special exception for this in the pipeline cleanup. Fixes: #32184 PR-URL: #32197 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

http1 objects are coupled with their corresponding res/req and cannot be treated independently as normal streams. Add a special exception for this in the pipeline cleanup. Fixes: #32184 Backport-PR-URL: #32212 PR-URL: #32197 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

ronag added the stream Issues and PRs related to the stream subsystem. label Mar 11, 2020

ronag requested a review from addaleax March 11, 2020 12:44

ronag requested a review from mcollina March 11, 2020 12:45

ronag added the v13.x label Mar 11, 2020

addaleax approved these changes Mar 11, 2020
View reviewed changes

addaleax removed the v13.x label Mar 11, 2020

mcollina approved these changes Mar 11, 2020
View reviewed changes

ronag force-pushed the pipeline-fix-destroyer branch 2 times, most recently from 5aeab8d to abd091cCompare March 11, 2020 13:35

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

BridgeAR reviewed Mar 11, 2020
View reviewed changes

Comment threadtest/parallel/test-stream-pipeline.js Outdated

MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 11, 2020

ronag added 3 commits March 11, 2020 16:51

stream: avoid destroying http1 objects
70eabc1
http1 objects are coupled with their corresponding res/req and cannot be treated independently as normal streams. Add a special exception for this in the pipeline cleanup. Fixes: nodejs#32184

fixup: don't depend on private properties
bed9a32

fixup: suggestions
1415464

ronag force-pushed the pipeline-fix-destroyer branch from d83675f to 1415464Compare March 11, 2020 15:51

ronag mentioned this pull request Mar 11, 2020
[v13.x backport] pipeline destroy compat #32212
Closed
4 tasks

MylesBorins closed this Mar 11, 2020

MylesBorins added the backport-open-v13.x label Mar 11, 2020

MylesBorins mentioned this pull request Mar 12, 2020
v13.11.0 proposal #32185
Merged

ronag mentioned this pull request Apr 20, 2020
stream: ensure pipeline always destroys streams #31940
Closed
4 tasks

mafintosh mentioned this pull request Apr 20, 2020
tar-stream regression in 13 with pipeline #32954
Closed

targos added the backport-blocked-v12.x label Apr 22, 2020

Uh oh!

Uh oh!

Conversation

ronag commented Mar 11, 2020

Checklist

Uh oh!

ronag commented Mar 11, 2020

Uh oh!

nodejs-github-bot commented Mar 11, 2020

Uh oh!

mcollina commented Mar 11, 2020

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Mar 11, 2020

Uh oh!

mcollina commented Mar 11, 2020

Uh oh!

nodejs-github-bot commented Mar 11, 2020

Uh oh!

Uh oh!

MylesBorins commented Mar 11, 2020

Uh oh!

ronag commented Mar 11, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

mcollina commented Mar 11, 2020

Uh oh!

MylesBorins commented Mar 11, 2020

Uh oh!

MylesBorins commented Mar 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

ronag commented Mar 11, 2020•
edited
Loading

nodejs-github-bot commented Mar 11, 2020•
edited by MylesBorins
Loading