http: initialization of request options

[nitroduna]

Documentation of http.Agent (https://github.com/joyent/node/blob/master/doc/api/http.markdown#new-agentoptions)
doesn't say you can use properties of the options object of the "request" method in the options object of the Agent "constructor" to configure default request options.

But the code is allowing when it extends the "options" object of requests with the "this.options" of the Agent in the "createSocket" method. And the documentation of "https.request"(https://github.com/joyent/node/blob/master/doc/api/https.markdown#httpsrequestoptions-callback) shows an example of creating a custom instance of "https.Agent" using SSL default request options such as key or cert.

The problem extending or modifying the request options object in the "createSocket" method is that "addRequest" method can end up calling the "getName" method with different data to those used by "createSocket" method. There are some lines in the code that calls "getName" and it's important to always use the same data in options object to make sure that the created key to access "sockets" or "freeSockets" maps is always the same, to reuse and release the sockets correctly.

If you create an "https.Agent" using an option object with for example a default "rejectUnauthorized" configuration, the name created by "getName" will be different the first time called by "addRequest" and the second time called by "createSocket" method.

    var options = {
        rejectUnauthorized: false
    };
    options.agent = new https.Agent(options);

Moving the initialization to 'addRequest' solves the problem.

[trevnorris]

/cc @tjfontaine

[stigerland]

any progress on this? I just got bitten by this where the agent.options contained options that httpOptions didn't include. It is easy enough to avoid the problem if you are aware of the bug, but still it shouldn't be necessary.

[trevnorris]

/cc @cjihrig @misterdjules

[misterdjules]

@nitroduna @trevnorris This seems to highlight a genuine problem, thank you!

However, the current state of this PR seems to assume that Agent.prototype.createSocket is always called from Agent.prototype.addRequest, which would be the only one responsible for merging self.options into the options object passed as a parameter.

Would it make sense to always merge self.options with the options parameter, to prevent other issues if one day Agent.prototype.createSocket is called from another caller than Agent.prototype.addRequest?

If so, we could also factor out this code into a separate function.

We would also need a test or a tests suite added to this PR to be able to merge it.

[jasnell]

@nitroduna ... what would you like to do with this? If you wish to pursue, the PR would need to be updated and reopened against master on http://github.com/nodejs/node.

[jgrund]

I've hit this as well.

[cjihrig]

@jgrund would you please open a new issue at nodejs/node, preferably with code to reproduce the problem.

[cjihrig]

Closing this as this repo is no longer active.

[wayneseymour]

I've hit this too.

[jgrund]

Sure, I'll move it over.

[jgrund]

Opened: nodejs/node#4652