Race condition in http.ClientRequest causes socket to hang up upon req.write(). #2827

c4milo opened this Issue Feb 25, 2012 · 10 comments


None yet

3 participants


I already described and discussed this issue with @mikeal in #2812. He agreed in that it may be a bug.


Summary, please correct me if I'm wrong.

Gist: https://gist.github.com/1889610

Problem: Chunks written to a socket which is used by an HTTP request in the on('socket') event will precede the header, resulting in an invalid HTTP request, and (probably) the server killing the connection.

Possible solutions:

  1. Document the behavior (somewhat unsatisfying, but 100% safe.)
  2. Don't emit socket event in an HTTP request as soon as the socket is assigned, but rather immediately after the headers etc. have been written to it. (Simple safe workaround, but somewhat inelegant. Change to behavior which could break programs. This is not allowed in v0.6, unless we recently changed the behavior from this for some reason, in which case, this would be a bugfix.)
  3. Add a second event socketReady or something that will be emitted once the socket is ready for writing (ie, after the headers are written.) New API, not allowed in v0.6.
  4. Start all/most streams in a "paused and buffering" state, and only unpause the http stream when the underlying TCP stream is ready to have HTTP body bits written to it. (Most elegant, but most work, and need to tread very carefully to get the performance characteristics right.)

it summarizes correctly the issue @isaacs. I am also more inclined towards number 2. It's just to make sure that the first req.write() sends the request along with headers and before the socket event is emitted.


Oh, actually, number 2 might not be an option in v0.6. It would break the optimization we have to send the response in a single packet when you do res.writeHead(200); res.end("bar");.

I'm inclined to just document this wart, and fix it properly in master.


I am fine having it documented. I'll be also extremely useful to have the events model or state machines documented somewhere, so that people can better understand how modules like net and http interact between each other through those events.


Actually, cause of this is net module not http. I will write the patch soon.

@koichik koichik added a commit to koichik/node that referenced this issue Feb 26, 2012
@koichik koichik net: fix race write() before and after connect()
Fixes #2827.

@isaacs Can you review koichik/node@f15885c (for v0.6 branch)?


boom!, that made it @koichik. Nicely done, it's working fine for me.


@koichik Looks good to me. Land at will.


@isaacs Thanks, merging.

@koichik koichik closed this in e0ab4ec Feb 28, 2012

@c4milo Thanks for the report, fixed in cd5d247 (master) and 6343179 (v0.6).

@shigeki shigeki pushed a commit to shigeki/node-v0.x-archive that referenced this issue Feb 28, 2012
@koichik koichik net: fix race write() before and after connect()
Fixes #2827.
@isaacs isaacs pushed a commit to isaacs/node that referenced this issue Feb 29, 2012
@koichik koichik net: fix race write() before and after connect()
Fixes #2827.
@isaacs isaacs added a commit to isaacs/node that referenced this issue Mar 2, 2012
@isaacs isaacs 2012.03.02 Version 0.6.12 (stable)
* Upgrade V8 to

* dtrace ustack helper improvements (Dave Pacheco)

* API Documentation refactor (isaacs)

* #2827 net: fix race write() before and after connect() (koichik)

* #2554 #2567 throw if fs args for 'start' or 'end' are strings (AJ ONeal)

* punycode: Update to v1.0.0 (Mathias Bynens)

* Make a fat binary for the OS X pkg (isaacs)

* Fix hang on accessing process.stdin (isaacs)

* repl: make tab completion work on non-objects (Nathan Rajlich)

* Fix fs.watch on OS X (Ben Noordhuis)

* Fix #2515 nested setTimeouts cause premature process exit (Ben Noordhuis)

* windows: fix time conversion in stat (Igor Zinkovsky)

* windows: fs: handle EOF in read (Brandon Philips)

* windows: avoid IOCP short-circuit on non-ifs lsps (Igor Zinkovsky)

* Upgrade npm to 1.1.4 (isaacs)
  - windows fixes
  - Bundle nested bundleDependencies properly
  - install: support --save with url install targets
  - shrinkwrap: behave properly with url-installed modules
  - support installing uncompressed tars or single file modules from urls etc.
  - don't run make clean on rebuild
  - support HTTPS-over-HTTP proxy tunneling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment