open timeout not cleared until read response

[starpit]

if i make a blocking request to a service that takes more than 10 seconds, needle will fail, in its default configuration, due to the default open timeout of 10 seconds.

it appears that the open timeout is not cleared until a read response is received. https://github.com/tomas/needle/blob/master/lib/needle.js#L479

my interpretation of "open timeout" is as relates to the time until a socket connection is established. this helps one distinguish network partitions or edge failures from service outages.

[tomas]

I'll take a look in a minute. Thanks for reporting!

[tomas]

You're right, open timeout should be cleared when the connection is established, rather than when the response headers are received. This is simply the result of Needle using the available events provided by Node's HTTP module.

There is, actually, a 'connect' event but it serves a totally different purpose. It simply fires whenever a server responds to a CONNECT type request.

Now it looks like the underlying net module does provide a real 'connect' event so I think we might be able to pull it off.

I'll dive into this later tonight. Thanks again for the heads up.

[tomas]

I think this should do it:

4b680a6

What do you think?

[starpit]

hi @tomas

echo test: this change establishes a cascade of timers:

1. on request, set a timer based on open_timeout
2. on socket.connect, clear the open_timeout timer, and set a new timer based on a new config property response_timeout
3. on response, clear the response_timeout timer,
   ... the rest of the logic remains unchanged

i think this looks good. it is also analogous to the accepted solution here: https://stackoverflow.com/a/9910413

that SO solution has one extra bit, i'm not sure if it's necessary, but it might be worth some reflection:

req.on('socket', function (socket) {
    socket.setTimeout(myTimeout);  
    socket.on('timeout', function() {  // <--- this part
        req.abort();
    });
});

i believe that this timeout event is only emitted for idle connections, e.g. those in connection pools. so it's probably not pertinent to needle?

[tomas]

No, we don't need to listen for 'timeout' events since we use the regular setTimeout instead of socket.setTimeout, which doesn't work consistently across different node.js versions.

I'm also thinking of adding a 'max_time' option, which would act as a global timer for the whole request/response process, although I'm not sure what the behaviour should be when following redirects. Any ideas?

[starpit]

i suggest we move the max_time discussion to a separate PR. it seems like a nice feature to have.

[tomas]

Ok, just published v2.1.0 with the updated behaviour. Thanks again!