Skip to content

Move timeout from the incoming socket to the outgoing one. #649

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

Closed
wants to merge 2 commits into from

Conversation

mdirolf
Copy link

@mdirolf mdirolf commented Jun 5, 2014

The issue was that when setting a timeout it was only getting set on the incoming socket, not on the outgoing http request. So the incoming request would get closed but the error handler wouldn't run, and the outgoing request would remain open.

This PR moves the timeout from the incoming socket to the outgoing one, and aborts the request on a timeout. The result is that error callbacks will run on a timeout, and that outgoing http requests won't remain open.

@josegonzalez
Copy link

Wouldn't you want both a request and a response timeout instead?

@mdirolf
Copy link
Author

mdirolf commented Jun 5, 2014

What is the use case for having a separate timeout on the incoming socket? With the above commit if the outgoing request times out the request aborts and the proxy server responds immediately.

@josegonzalez
Copy link

First, this would be a backwards incompatible change. Second, you could have a client that opens a connection but doesnt actually send a request to the node proxy, which is what I think the original feature covers.

@mdirolf
Copy link
Author

mdirolf commented Jun 5, 2014

Fair enough - I'll change the PR to just add a new option to set a timeout for the outgoing http request.

@jcrugzz
Copy link
Contributor

jcrugzz commented Jun 10, 2014

@mdirolf I like this, could you just change the proxy_timeout to proxyTimeout. My OCD wants camelCase ;). Otherwise LGTM

@Rush
Copy link
Contributor

Rush commented Jun 10, 2014

Okay seems like my pull request #658 seems to achieve the same thing. I have timeout and targetTimeout there, if this pull request is gonna be pulled please take the test for original timeout as well.

@Rush
Copy link
Contributor

Rush commented Jun 10, 2014

Additionally the test for socket.io needs to be fixed, for which my pull request also has a change.

@jcrugzz
Copy link
Contributor

jcrugzz commented Jun 10, 2014

@RushPL yes i still wanted to steal your tests :). It all comes down to the naming so I will pull whatever PR that uses a proxyTimeout naming as they do accomplish the same thing.

@jcrugzz
Copy link
Contributor

jcrugzz commented Jun 10, 2014

fixed with #658

@jcrugzz jcrugzz closed this Jun 10, 2014
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

Successfully merging this pull request may close these issues.

4 participants