Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

tunnel.removeSocket needs to be overridden for secure Sockets #2

Closed
frooble opened this issue May 24, 2012 · 1 comment
Closed

tunnel.removeSocket needs to be overridden for secure Sockets #2

frooble opened this issue May 24, 2012 · 1 comment

Comments

@frooble
Copy link

frooble commented May 24, 2012

For httpsOverHttp and httpsOverHttps tunnels, createSocket method is replaced with createSecureSocket which adds the actual socket as a member of the object returned from tls.connect. Subsequent processing places listeners on this tls connection object and not on the underlying socket. When these listener callbacks invoke removeSocket, they end up providing the tls.connect object as an argument and not the actual underlying socket.

The removeSocket looks for a match of the provided tls.connect object to an element in the array in order to find the one to remove. However, since the actual underyling transport socket is what is placed in the sockets member array, no match is found, nothing is removed, and eventually the sockets array fills up to the maxSockets length and subsequent requests will be queued permanently.

Recommending providing a removeSecureSocket method

function removeSecureSocket (secureSocket) {
    var self = this; 
    TunnelingAgent.prototype.removeSocket.call(self, secureSocket);
}

And then replacing the removeSocket method for httpsOverHttp and httpsOverHttps instances with the secure socket version (essentially mirroring what is done for createSocket)

function httpsOverHttp(options) {
  var agent = new TunnelingAgent(options);
  agent.request = https.request;
  agent.createSocket = createSecureSocket;
  agent.removeSocket = removeSecureSocket;
  return agent;
}

function httpsOverHttps(options) {
  var agent = new TunnelingAgent(options);
  agent.request = https.request;
  agent.createSocket = createSecureSocket;
  agent.removeSocket = removeSecureSocket;
  return agent;
}
@koichik
Copy link
Owner

koichik commented May 26, 2012

@frooble - Thanks! Fixed in da4c509.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants