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: fixed invalid path with connect method #34408

Closed
wants to merge 1 commit into from
Closed

http: fixed invalid path with connect method #34408

wants to merge 1 commit into from

Conversation

preyunk
Copy link
Contributor

@preyunk preyunk commented Jul 17, 2020

Fixes: #34347

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 17, 2020
@preyunk
Copy link
Contributor Author

preyunk commented Jul 17, 2020

@szmarczak

lpinca
lpinca requested changes Jul 17, 2020
View reviewed changes
lib/_http_client.js
@@ -387,7 +389,7 @@ function _destroy(req, socket, err) {
if (err) {
req.emit('error', err);
}
req._closed = true;
req[kClosed] = true;
Copy link
Member

Choose a reason for hiding this comment

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

This is not ok, see discussion on #34313.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems strange, I can't remember changing this part of the file however I will revert the changes for now and create another PR.

Copy link
Member

Choose a reason for hiding this comment

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

git pull --rebase

@szmarczak
Copy link
Member

Also this needs tests.

@preyunk preyunk closed this Jul 17, 2020
@preyunk preyunk deleted the connect-pathname branch July 17, 2020 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@szmarczak szmarczak szmarczak left review comments

@lpinca lpinca lpinca requested changes

Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Passing a URL instance with a CONNECT method results in an invalid path
4 participants
@preyunk @szmarczak @lpinca @nodejs-github-bot