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

https: Agent.createConnection mutates options object #31119

Closed
ronag opened this issue Dec 28, 2019 · 9 comments
Closed

https: Agent.createConnection mutates options object #31119

ronag opened this issue Dec 28, 2019 · 9 comments
Labels
good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem.

Comments

@ronag
Copy link
Member

ronag commented Dec 28, 2019

createConnection should create a copy instead of mutating the passed options object.

const options = { port: 3000 };
htptpAgent.createConnection('localhost', 2000, options);
assert(options.port, 3000); // fails
assert(options.host, undefined); // fails
@ronag
Copy link
Member Author

ronag commented Dec 28, 2019

Good first issue?

@ronag ronag changed the title https: createConnection mutates options object https: Agent.createConnection mutates options object Dec 28, 2019
@Trott Trott added good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem. labels Dec 30, 2019
@vighnesh153
Copy link
Contributor

vighnesh153 commented Jan 1, 2020

Hey @ronag , @Trott . I would like to go fix this issue. I have read the CONTRIBUTING.md file. Is there anything else I need to know before proceeding like should I just create a pull request after solving the issue or do I have to do something else as well? This is my first time contributing to an open-source project. Any help would be appreciated.

@ronag
Copy link
Member Author

ronag commented Jan 1, 2020

@vighnesh153: Create a PR and follow the instructions. Also please add a test that fails before fix and succeeds after fix.

@vighnesh153
Copy link
Contributor

Alright. I will start working on it right away.

@vighnesh153
Copy link
Contributor

In the lib/_http_agent.js, I can see a line:
Agent.prototype.createConnection = net.createConnection;
That leads me to the lib/net.js file. There, I saw

module.exports = {
...,
  createConnection: connect,
...
}

So, from there, I went to connect definition and its comment-doc says that it has 3 forms:

// There are various forms:
//
// connect(options, [cb])
// connect(port, [host], [cb])
// connect(path, [cb]);

None of them matches the one that is mentioned in the issue. Am I looking at the correct function?

Also, I see a test directory and inside it, there are several other directories. One of them being internet. Should I add a new test file in that directory or should I use an existing one?

@ronag
Copy link
Member Author

ronag commented Jan 1, 2020

You should be looking for https not http.

See the test in test/parallel for examples and you should either add a new test there or modify and existing one.

@vighnesh153
Copy link
Contributor

Ok. Got it. I will look into that.

@vighnesh153
Copy link
Contributor

vighnesh153 commented Jan 1, 2020

While running the tests, some of the tests pass, but many others throw unhandled errors. I haven't touched the code yet. Am I missing out on something?

Edit:
Those were some experimental features. I guess I should ignore those errors.

@vighnesh153
Copy link
Contributor

@ronag This is the PR link. It is a bit messy but I have squashed all commits into one at the end. Please give your feedback on it. #31151

@Trott Trott closed this as completed in fa94698 Jan 4, 2020
targos pushed a commit that referenced this issue Jan 6, 2020
Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119

PR-URL: #31151
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Jan 14, 2020
Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119

PR-URL: #31151
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
Previously, when passing options object to the agent.createConnection
method, the same options object got modified within the method. Now,
any modification will happen on only a copy of the object.

Fixes: #31119

PR-URL: #31151
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. https Issues or PRs related to the https subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants