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

http: end with data should emit write after end error #28666

Closed
wants to merge 2 commits into from
Closed

http: end with data should emit write after end error #28666

wants to merge 2 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 13, 2019 •
edited
Loading

Calling end() with data while ending should trigger a write after end error.

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

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 13, 2019
@ronag
Copy link
Member Author

ronag commented Jul 13, 2019

@benjamingr ping

@ronag ronag changed the title http: end with data can cause write after end http: end with data should emit write after end error Jul 13, 2019
@ronag
Copy link
Member Author

ronag commented Jul 13, 2019 •
edited
Loading

Never mind write_ already handles this.

@ronag ronag closed this Jul 13, 2019
@ronag ronag reopened this Jul 13, 2019
@ronag ronag force-pushed the fix-http-write-after-end branch 2 times, most recently from f3e0175 to 13b9ff3 Compare July 13, 2019 12:19
@lpinca
Copy link
Member

lpinca commented Jul 13, 2019

SGTM, please add a test.

@lpinca lpinca added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 13, 2019
@ronag ronag force-pushed the fix-http-write-after-end branch 2 times, most recently from 78a3f2e to 0560050 Compare July 13, 2019 13:00
@ronag
Copy link
Member Author

ronag commented Jul 13, 2019

@lpinca added

@nodejs-github-bot
Copy link
Collaborator

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

@benjamingr
Copy link
Member

Sure this seems reasonable, can we can a citgm run?

benjamingr
benjamingr approved these changes Jul 13, 2019
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.

LGTM with citgm run :)

@lpinca
Copy link
Member

lpinca commented Jul 14, 2019

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1904/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Jul 16, 2019
@Trott
Copy link
Member

Trott commented Jul 16, 2019

Added blocked label on the grounds that CITGM results will be difficult to sort through until standard-things/esm#821 is resolved.

@ronag
Copy link
Member Author

ronag commented Aug 9, 2019

@Trott: This is no longer blocked

@Trott Trott removed the blocked PRs that are blocked by other issues or PRs. label Aug 9, 2019
@ronag
Copy link
Member Author

ronag commented Aug 17, 2019

@Trott: this looks ready

@nodejs-github-bot
Copy link
Collaborator

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

@Trott
Copy link
Member

Trott commented Dec 14, 2019

@Trott: How's the CITGM?

Hmmm...I botched the link. Let's try again...

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2129/ (queued)

@ronag
Copy link
Member Author

ronag commented Dec 26, 2019

@Trott: Can we land this?

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 30, 2019 •
edited by Trott
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/28030/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2169/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@ronag
Copy link
Member Author

ronag commented Jan 11, 2020

@Trott: flaky CI? failure seems unrelated

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

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

@Trott
Copy link
Member

Trott commented Jan 12, 2020

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2220/

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 12, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@ronag
Copy link
Member Author

ronag commented Jan 27, 2020 •
edited
Loading

@Trott: Was CITGM fine (404 now)? Can this land?

@ronag

This comment has been minimized.

Sign in to view
@ronag

This comment has been minimized.

Sign in to view
@ronag
Copy link
Member Author

ronag commented Feb 14, 2020

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2265/

@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

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

ronag added a commit that referenced this pull request Feb 16, 2020
@ronag
http: end with data can cause write after end
c776a37 
Calling end() with data while ending should trigger
a write after end error.

PR-URL: #28666
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ronag
Copy link
Member Author

ronag commented Feb 16, 2020 •
edited
Loading

Landed in aca879be160d
Landed in c776a37

@ronag ronag closed this Feb 16, 2020
@sthagen sthagen mentioned this pull request Feb 16, 2020
Merged
4 tasks
@targos
Copy link
Member

targos commented Feb 16, 2020

Looks like it landed in c776a37 instead

@BethGriggs BethGriggs mentioned this pull request Mar 10, 2020
Merged
@bessw bessw mentioned this pull request Sep 23, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BridgeAR BridgeAR BridgeAR left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@trivikr trivikr trivikr approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@ronag @lpinca @nodejs-github-bot @benjamingr @Trott @targos @mcollina @jasnell @BridgeAR @trivikr