Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Fixes #1399 #1400

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
4 participants

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

Owner

bnoordhuis commented Jul 26, 2011

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?

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.

@koichik: any thoughts?

Member

mikeal commented Aug 1, 2011

did you try this with --use-http2?

@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.

Member

mikeal commented Aug 1, 2011

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.

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.

Member

mikeal commented Aug 1, 2011

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 commented Aug 2, 2011

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?

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

@SaltwaterC SaltwaterC closed this Nov 7, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment