getName in addRequest returns wrong value when using legacy API

[encojosh]

// Legacy API: addRequest(req, host, port, path)
if (typeof options === 'string') {
    options = {
      host: options,
      port: arguments[2],
      path: arguments[3]
    };
    // Need to add the following lines to correct the problem
    options = util._extend({}, options);
    options = util._extend(options, this.options);

    options.servername = options.host;
    if (req) {
        var hostHeader = req.getHeader('host');
        if (hostHeader) {
            options.servername = hostHeader.replace(/:.*$/, '');
        }
    }
}

[encojosh]

I should note that the existing code base works fine for http but not https.

[wraithan]

This library is meant to mimic the behavior of the 0.12 http agent, but be adapted to run everywhere. It started as a copy from the 0.11.x source and then was modified to run on more versions of node.

https://github.com/nodejs/node/blob/v0.12/lib/_http_agent.js#L133-L175

I do see how it is different than the 0.10 version of the same thing:

https://github.com/nodejs/node/blob/v0.10/lib/http.js#L1260-L1278

which uses createSocket which does what you are asking for.

Not sure what the "right" behavior is for this.

[wraithan]

If you could better describe the problem you are encountering (especially with a reproduction and what version of node you are on), we can have a more detailed discussion.

[encojosh]

I am running v0.10.38. When sending HTTPS requests, socket pooling was not working. Basically, the getName function in the addRequest function returns host:port:::::: because the HTTPS options are not in the options object. All other references to getName pass in the options object that contains the HTTPS options. Those references return hostname:port:cert:ciphers:key:pfx Therefore, when looking up the freeLen and sockLen on lines 176 & 177, always returns 0. This forces a new socket to be created each time a HTTPS request is made.

[encojosh]

Another way and I think better way to fix it would be to modify the HTTPs getName function.

SSLAgent.prototype.getName = function(options) {
var name = Agent.prototype.getName.call(this, options);

// name += ':';
// if (options.ca)
//   name += options.ca;
// else if (typeof this.options === "object" && this.options && this.options.ca)
//   name += this.options.ca;

name += ':';
if (options.cert)
    name += options.cert;
else if (typeof this.options === "object" && this.options && this.options.cert)
    name += this.options.cert;

name += ':';
if (options.ciphers)
    name += options.ciphers;
else if (typeof this.options === "object" && this.options && this.options.ciphers)
    name += this.options.ciphers;

name += ':';
if (options.key)
    name += options.key;
else if (typeof this.options === "object" && this.options && this.options.key)
    name += this.options.key;

name += ':';
if (options.pfx)
    name += options.pfx;
else if (typeof this.options === "object" && this.options && this.options.pfx)
    name += this.options.pfx;

// name += ':';
// if (!isUndefined(options.rejectUnauthorized))
//   name += options.rejectUnauthorized;
// else if (typeof this.options === "object" && this.options && this.options.rejectUnauthorized)
//   name += this.options.rejectUnauthorized;

return name;

};

[wraithan]

Thanks for the extra detail. I'll go though the 0.10 and 0.12 sources and see if I can't distill an implementation that works better for you while still staying true to what core does.

I don't have time to do this today unfortunately, might be able to look into it this weekend.