Skip to content

Revert "http: align with stream.Writable"#37963

Closed
mcollina wants to merge 1 commit intonodejs:masterfrom
mcollina:revert-36816
Closed

Revert "http: align with stream.Writable"#37963
mcollina wants to merge 1 commit intonodejs:masterfrom
mcollina:revert-36816

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Mar 29, 2021

This reverts commit e2f5bb7.

Reverted as it caused a significant performance regression.

See: #37937

@mcollina
Revert "http: align with stream.Writable"
21db5dd 
This reverts commit e2f5bb7.

Reverted as it caused a significant performance regression.

See: nodejs#37937
@mcollina mcollina requested review from jasnell and ronag March 29, 2021 11:01
@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Mar 29, 2021
@mcollina
Copy link
Member Author

mcollina commented Mar 29, 2021

cc @BethGriggs this needs to go in v16.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 29, 2021

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

@mcollina
Copy link
Member Author

mcollina commented Mar 29, 2021

cc @nodejs/http

@mcollina mcollina mentioned this pull request Mar 29, 2021
Closed
3 tasks
benjamingr
benjamingr approved these changes Mar 29, 2021
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bummer. Happy we tried.

@benjamingr
Copy link
Member

benjamingr commented Mar 29, 2021

@nodejs/streams

@ronag
Copy link
Member

ronag commented Mar 29, 2021 •
edited
Loading

Is the test in #37964 sufficient to avoid mem leak regression? Or was that not caused by this PR?

@ronag
Copy link
Member

ronag commented Mar 29, 2021 •
edited
Loading

Bummer. Happy we tried.

We will try again 💪! (in the future)

@mcollina
Copy link
Member Author

mcollina commented Mar 29, 2021

Is the test in #37964 sufficient to avoid mem leak regression? Or was that not caused by this PR?

That's a close map to what autocannon was doing, it should be sufficient. However, autocannon was not causing warning with this reverted but the test still failed, so there might be something I'm missing.

@ronag
Copy link
Member

ronag commented Mar 29, 2021

That's a close map to what autocannon was doing, it should be sufficient. However, autocannon was not causing warning with this reverted but the test still failed, so there might be something I'm missing.

Can we use autocannon in a test?

@mcollina
Copy link
Member Author

mcollina commented Mar 29, 2021

@nodejs/testing @nodejs/build is the test-asan job supposed to pass here or can this land without?

@mcollina mcollina mentioned this pull request Mar 29, 2021
Closed
mcollina added a commit that referenced this pull request Mar 31, 2021
@mcollina
Revert "http: align with stream.Writable"
16920db 
This reverts commit e2f5bb7.

Reverted as it caused a significant performance regression.

See: #37937

PR-URL: #37963
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mcollina
Copy link
Member Author

mcollina commented Mar 31, 2021

Landed in 16920db

@mcollina mcollina closed this Mar 31, 2021
@mcollina mcollina deleted the revert-36816 branch March 31, 2021 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@ronag ronag ronag approved these changes

@BethGriggs BethGriggs BethGriggs approved these changes

@jasnell jasnell Awaiting requested review from jasnell

+1 more reviewer

@dnlup dnlup dnlup approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@mcollina @nodejs-github-bot @benjamingr @ronag @lpinca @cjihrig @BethGriggs @dnlup @MylesBorins