Don't abort request, if it returned an error status but is otherwise fine

[stefanklug]

Hi I stumbled upon the following problem:
I use this module inside a cache seeder which sends lots of GET requests to a cache. In some cases the cache correctly answers with several thousands of 404s.
As http-request always calls request.abort() in an error case (even though there wasn't really an error, just the response code is 404), nodejs closes the socket and doesn't use it with keepAlive connections. That resulted in my server running out of sockets/ports.

The patch isn't really nice. The only place where I really wanted to prevent request.abort() is in request.js line 432:

stream.on('end', function() {
    err.document = Buffer.concat(buf, size);
    self.error(err, callback); //request mustn't abort here
});

So any advice on a better solution is greatly appreciated.

Cheers Stefan

[SaltwaterC]

Hi Stefan,

I've to admit that I haven't checked the persistent connection behaviour as I wrote this library for a service that uses lots of different URLs (a file mirroring service to be more precise). Persistence was never an issue as with 100k+ URLs things change from one request to another.

This seems to be the case of throwing the baby out with the bathwater. Aborting a request shouldn't really close a socket if a persistent connection is being used, but this is something which needs to be addressed by node.js itself which I don't think is going to help you much. I'll have to give it a run as I see Travis is complaining about your merge.

One of the reasons for aborting the request is the response buffering for error responses. For keeping the client from crashing with an OOM error if the response body is huge, but an error status is returned, the client buffers up to 1MiB of data. This may not be the case, but I can't fix this case as the request needs to be aborted prematurely.

Everything from the 404 branch is wrapped by Request.prototype.error, even when the client stream reaches end event. I guess the persistent socket keeps the request object to be truthful (see request.js). I guess a cleaner fix is to pass the stream event to Request.prototype.error to keep it from aborting when it reaches end. However, it would only work when a 404 reaches a clean end event.

Request.prototype.error itself should avoid any race conditions in this case. Race conditions were the reason for things that may seem to be bit over-engineered with so many wrappers, but I had my fair share of race conditions in node.js (even closed a few upstream race conditions). The error and finished states are the safeguards against potential race conditions.

Cheers,
Ștefan