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

test suite fails w/ ECONNREFUSED #48

Closed
jfis opened this issue May 10, 2011 · 14 comments
Closed

test suite fails w/ ECONNREFUSED #48

jfis opened this issue May 10, 2011 · 14 comments

Comments

@jfis
Copy link

jfis commented May 10, 2011

I'm running into issues that seem like a problem with just my system but I was hoping to find some assistance.

node 0.4.7
http-proxy 0.5.0
vows 0.5.8
colors 0.5.0
optimist 0.2.0
request 1.9.5
socket.io 0.6.17
on mac os x 10.6.7

When I run

 vows test/*-test.js --spec

all tests fail that are not expecting a 404 or 500.

The errors look like this:

When using server created by httpProxy.createServer() with no latency and a valid target server
✗ should receive 'hello localhost'
» expected 'hello localhost',
got  'An error has occurred: {"stack":"Error: ECONNREFUSED, Connection refused\\n    
at Socket._onConnect (net.js:599:18)\\n    at IOWatcher.onWritable [as callback 
(net.js:186:12)","message":"ECONNREFUSED, Connection      
refused","errno":61,"code":"ECONNREFUSED","syscall":"connect"}' (==) // helpers.js:77

Anyone seen this before? Seems like I'm missing something important. There is very little information on ECONNREFUSED. I do know that the error happens on making the request from proxy to target using the 'request' module (which is expected given the error) but I can't determine why. I'm hoping there's something simple I missed.

@Marak
Copy link
Contributor

Marak commented May 10, 2011

ECONNREFUSED is a generic node.js error message when a socket cannot make a connection. It usually indicates that the listening server isn't infact listening, or the outgoing request you made to the server could not find its target.

With that being said, I'm not entirely sure what your issue is. Could it be possible you are already running services on the port http-proxy is trying to use for it's tests? Maybe you could change the port number in the test files?

@indexzero - Do you have any ideas why this could be happening?

@jfis
Copy link
Author

jfis commented May 10, 2011

Thanks for the response. The ports are already varied so every test shouldn't fail if it had to do with port. I took a shot anyway and I did mix them up but no change.

To add to this mystery... I get the exact same results on my ubuntu 10.10 vps with the same node, npm, and module versions.

@indexzero
Copy link
Contributor

What versions of node and npm are you running?

@indexzero
Copy link
Contributor

I just ran the test suite locally on my machine, and I can confirm I am seeing the same errors in the test suite. Right now I'm going to assume that it's a regression from an update to node.js core.

I'm in SF until the weekend, so I may not have time to properly address this until Monday.

@jfis
Copy link
Author

jfis commented May 10, 2011

No worries on time table.
I'm just glad someone else can replicate.

I see I forgot npm in the original.

node 0.4.7
npm 1.0.6
http-proxy 0.5.0
vows 0.5.8
colors 0.5.0
optimist 0.2.0
request 1.9.5
socket.io 0.6.17

@indexzero
Copy link
Contributor

I tried to reverting back to node v0.4.6 and I got the same results. I'm on HEAD right now though, so I'm going to revert back to individual commits until this is passing. I always ensure the entire test suite passes before any release so it's strange that this regression popped up here on a stable tag

@indexzero
Copy link
Contributor

I can confirm that on v0.4.6 the following commit passes all HTTP tests: ddf31b2. I'm upgrading back to v0.4.7 to test this as well now. This may be a bad rebase in node.js core because the only difference here is the underlying agent API change from this pull request: #39

@jfis
Copy link
Author

jfis commented May 10, 2011

{protocol}.getAgent() method signature
http://nodejs.org/docs/v0.4.7/api/http.html#http.getAgent
(I don't see getAgent for https in the docs?)

If I change these lines in node-http-proxy.js:

function _getAgent (host, port, secure) {
//var options = { host: host, port: port };
//var agent = !secure ? http.getAgent(options) : https.getAgent(options);
var agent = !secure ? http.getAgent(host, port) : https.getAgent(host, port);

agent.maxSockets = maxSockets;
return agent;
}

Everything seems to work then.

Then I found this haha
nodejs/node-v0.x-archive#943

@jfis
Copy link
Author

jfis commented May 10, 2011

And there's already a pull request

#45

@indexzero
Copy link
Contributor

Something is wrong in core. The method signature you suggest is not the correct method signature. This may be the result of a bad rebase on behalf of @ry.

In master both https.js and http.js indicate that both .getAgent() methods take an options hash, not a host, port pair, but in v0.4.7 it is still inconsistent. However, there is a commit from @mikeal rebased into February that actually fixes this.

Old commit that fixes this
nodejs/node-v0.x-archive@2b03ba5#lib/http.js

v0.4.7
https.js: https://github.com/joyent/node/blob/v0.4.7/lib/https.js#L70
http.js: https://github.com/joyent/node/blob/v0.4.7/lib/http.js#L1408

master
http.js: https://github.com/joyent/node/blob/master/lib/http.js#L1418
https.js: https://github.com/joyent/node/blob/master/lib/https.js#L80

I would like to suggest a hotfix release for nodejs as v0.4.7-1 or v0.4.8 to resolve this inconsistency. If that's not a good plan I can always just revert node-http-proxy to the original code under v0.5.0 that works around this inconsistency.

This issue should also be closed: nodejs/node-v0.x-archive#943 and is discussed more: #38

@jfis
Copy link
Author

jfis commented May 10, 2011

Yea I meant that signature only for comparison. It's what will work with 0.4.7.
Options hash makes more sense.

I agree that node-http-proxy should not be modified.

It's just good to know about this incompatibility if others have issues.

Thanks all.

@jfis jfis closed this as completed May 10, 2011
@indexzero
Copy link
Contributor

I reverted 642e158 in 40dc9de and rolled this up into 0.5.1. Confirmed all tests passing on node v0.4.7. I will address the above discussion when the next version of node is released.

@mikeal
Copy link

mikeal commented May 10, 2011

the fix only went in to master, not in to 0.4.x, because while this is a "bug" the fix for the bug makes 0.4.future not forward compatible.

@indexzero
Copy link
Contributor

@mikeal Thanks for clarifying. In master now there is a backwards compatible change that checks the type of the arguments passed to .getAgent(). Would love to see a hotfix version pushed out from @ry, but now that I've reverted it's not a big deal.

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

No branches or pull requests

4 participants