Fixes #1399

[SaltwaterC]

#1399 - HTTP client request.abort() race condition for requests with bodies

[bnoordhuis]

I think this is handled correctly in @mikeal's new http library (--use-http2 on master) where end() after abort() raises an error:

$ ./node --use-http2 test/simple/test-http-client-abort3.js

node.js:195
        throw e; // process.nextTick error, or 'error' event on first tick
              ^
Error: socket hang up
    at Socket.<anonymous> (http2.js:1143:25)
    at Socket.emit (events.js:88:20)
    at Array.<anonymous> (net_legacy.js:845:12)
    at EventEmitter._tickCallback (node.js:187:26)

@ry - thoughts?

[SaltwaterC]

In this http library for GET or HEAD requests calling end() after abort() doesn't do anything. You have to actually write() in order to trigger this condition. Therefore, for the sake of consistency I made the above statements.

This little bugger gave me a hard time with PUT requests on AWS S3 in certain rare conditions. This is the code that triggers the race condition:

var bfile = fs.ReadStream(bf);
bfile.on('data', function (chunk) {
    request.write(chunk);
});
bfile.on('end', function () {
    request.end();
});
bfile.on('error', function (error) {
    request.abort();
    callback(error, {});
});

This looks a lot like the code from #796. It only means that the read stream sometimes emits both the error and the end event. This has several implications that you may give some thought.

[SaltwaterC]

@koichik: any thoughts?

[mikeal]

did you try this with --use-http2?

[SaltwaterC]

@mikeal: as @bnoordhuis said, in this case the request error event is emitted. But there's no stable v0.5.x and no http2 in 0.4.x. As previously stated, calling end() after abort() does nothing. Calling end after write() + abort() crashes the http lib. I proposed this solution for the consistency reasons. It doesn't feel normal to have a different behavior for the same lib.

[mikeal]

You mean different behavior between 0.4.x and 0.5.x? The plan is to enable --use-http2 by default prior to the 0.6.0 release. There are API changes between the two http clients so having this difference wouldn't be the end of the world.

But, if this applies against 0.4.x we should merge it for the next bugfix release.

[SaltwaterC]

I mean different behavior into http.

request.abort();
request.end();

does not fail on http.js while

request.write(foo);
request.abort();
request.end()

fails on http.js.

Both of the above lines fail with "Error: socket hang up" on http2.js. http.js should pick one of these behaviors.

[mikeal]

http2 will be http before the next release, so if the goal is only to fix this for 0.6.0 then it's already in once we default to --use-http2

[koichik]

I am +1 to fix this on v0.4.
But I think end() should check it first like this:

OutgoingMessage.prototype.end = function(data, encoding) {
  if (this.finished || !this.writable) {
    return false;
  }

Unfortunately OutgoingMessage.writable is always true.
So I want to add this.writable = false to:

• OutgoingMessage.destroy()
• OutgoingMessage.end()
• ClientRequest.abort()

How about this?

[SaltwaterC]

Well, since 0.6 is the new stable, I guess this won't get fixed in 0.4 anyway.