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.

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

http: read request even after response is closed #28678

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jul 14, 2019

Trying to fix #27916 (comment)

I'm a little on deep water here and might need some guidance...

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 14, 2019
@ronag ronag force-pushed the fix-req-after-res branch 3 times, most recently from feea228 to 647c895 Compare July 14, 2019 11:52
@ronag
Copy link
Member Author

ronag commented Jul 14, 2019

@mcollina I'm a little confused in regards to the case where a request is aborted which even before this change seems to end with resOnFinish never being called?

@mcollina
Copy link
Member

Would you mind adding unit tests/example code for what you are trying to accomplish here?

@mcollina
Copy link
Member

This change is going to expose Node to a DoS attack because https://github.com/nodejs/node/pull/28678/files#diff-feaf3339998a19f0baf3f82414762c22R614 might not be called.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

As @mcollina suggests, this is not a great change. -1 on it.

@ronag
Copy link
Member Author

ronag commented Aug 23, 2019

I feel there is more to this but I think the discussion can continue in the issue. Closing this PR.

@ronag ronag closed this Aug 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ECONNRESET after response
4 participants