Skip to content
This repository

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

Closed
c4milo opened this Issue February 25, 2012 · 10 comments

3 participants

Camilo Aguilar Koichi Kobayashi Isaac Z. Schlueter
Camilo Aguilar

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

Isaac Z. Schlueter
Collaborator

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.)
Camilo Aguilar

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.

Isaac Z. Schlueter
Collaborator

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.

Camilo Aguilar

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.

Koichi Kobayashi
Collaborator

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

Koichi Kobayashi koichik referenced this issue from a commit in koichik/node February 27, 2012
Koichi Kobayashi net: fix race write() before and after connect()
Fixes #2827.
f15885c
Koichi Kobayashi
Collaborator

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

Camilo Aguilar

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

Isaac Z. Schlueter
Collaborator

@koichik Looks good to me. Land at will.

Koichi Kobayashi
Collaborator

@isaacs Thanks, merging.

Koichi Kobayashi koichik closed this issue from a commit February 28, 2012
Commit has since been removed from the repository and is no longer available.
Koichi Kobayashi koichik closed this in e0ab4ec February 28, 2012
Koichi Kobayashi
Collaborator

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

Shigeki Ohtsu shigeki referenced this issue from a commit in shigeki/node February 27, 2012
Koichi Kobayashi net: fix race write() before and after connect()
Fixes #2827.
cd5d247
Isaac Z. Schlueter isaacs referenced this issue from a commit in isaacs/node February 27, 2012
Koichi Kobayashi net: fix race write() before and after connect()
Fixes #2827.
6343179
Isaac Z. Schlueter isaacs referenced this issue from a commit March 02, 2012
Commit has since been removed from the repository and is no longer available.
Isaac Z. Schlueter isaacs referenced this issue from a commit March 02, 2012
Commit has since been removed from the repository and is no longer available.
Isaac Z. Schlueter isaacs referenced this issue from a commit March 02, 2012
Commit has since been removed from the repository and is no longer available.
Isaac Z. Schlueter isaacs referenced this issue from a commit March 02, 2012
Commit has since been removed from the repository and is no longer available.
Isaac Z. Schlueter isaacs referenced this issue from a commit in isaacs/node March 02, 2012
Isaac Z. Schlueter 2012.03.02 Version 0.6.12 (stable)
* Upgrade V8 to 3.6.6.24

* 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
48a2d34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.