Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

mcollina
Copy link
Member

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

cc @BethGriggs this needs to go in v16.

@nodejs-github-bot
Copy link
Collaborator

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

@mcollina
Copy link
Member Author

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

@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

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

@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

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

@dnlup dnlup dnlup approved these changes

@jasnell jasnell Awaiting requested review from jasnell

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