Fix issue with destroying proxy agent #46
Conversation
Proxy agent doesn't support destroy function
| this._proxyAgent.destroy(); | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
what about keep alives? Does proxy support that?
There was a problem hiding this comment.
From the testing I didn't it seemed to reuse sockets, but at https://github.com/koichik/node-tunnel/blob/master/lib/tunnel.js I don't see any keepAlive. I will have to retest to validate. I can try it out on Friday.
There was a problem hiding this comment.
Ahh it is indeed getting assigned at https://github.com/koichik/node-tunnel/blob/master/lib/tunnel.js#L47
There was a problem hiding this comment.
I also asked Ting to test and chime in on Proxy since he did most of that work.
There was a problem hiding this comment.
@TingluoHuang keepalive will still work if maxSocket is Infinity. maxSockets is for throttling requests. In node agent (https://nodejs.org/api/http.html#http_class_http_agent) the default value of maxSockets is Infinity, but I am ok with restricting it to 100 here.
proxyAgent doesn't support destroy function, so we don't have a choice.
There was a problem hiding this comment.
@stephenmichaelf it's possible if you are using proxy at the same time.
@omeshp i think there might be something wrong in the tunnel lib which cause the problem.
There was a problem hiding this comment.
@TingluoHuang I didn't get the issue stephen is facing. Are you saying that keepAlive isn't honored in case of proxy if maxSockets isn't set?
There was a problem hiding this comment.
@TingluoHuang @stephenmichaelf I think tunnel agent maintains list of sockets and reuses them (https://github.com/koichik/node-tunnel/blob/master/lib/tunnel.js#L58) by default, so sending keepAlive in proxy case won't matter, but this PR should go through since destroy function isn't supported in tunnel agent and users will get error while calling dispose on typed-rest-client.
There was a problem hiding this comment.
@TingluoHuang Maybe this is why we were seeing unexpected keep-alive results? Since this code is using tunnel and not the standard node sockets.
@omeshp To clarify, this is different than node's standard usage of sockets. We had some issues adding NTLM to this library due to node dropping sockets if there wasn't a request queued up on the socket when the initial request finished. I think this unique functionality in tunnel to reuse sockets is why this code is working. In NTLM we ended up using callbacks and setImmediate. Are you familiar with this? Is this what you would expect?
|
@TingluoHuang for review and proxy testing |
|
@TingluoHuang @omeshp Where are we on this? |
|
released as 1.0.6 |
Proxy agent doesn't support destroy function