Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

XMLHttpRequest piped in a writable file stream hangs next request #2263

Closed
gabipetrovay opened this Issue · 9 comments

3 participants

@gabipetrovay

Hi,

This is the thread where I came across this problem:
http://groups.google.com/group/nodejs/browse_thread/thread/5691a9fd25a6a65d

In that thread there was also another bug of mine but now I managed to minimize the remaining problem here:
http://dl.dropbox.com/u/2161705/test.zip

  1. Run the test app and open http://localhost.com:3000/test.html in your browser.
  2. An alert should tell you that an XMLHttpRequest POST request was made in the background.
  3. Click the Reload link and the page should hang (2 minutes in Chrome or Firefox)

I also paste the code here if somebody can directly see a flaw in it:

app.js

var express = require('express');

var app = module.exports = express.createServer();

app.configure(function(){
  app.use(app.router);
  app.use(express.static(__dirname + '/public'));
});

app.post('/test', function(req, res) {
    var fs = require('fs');
    var fileStream = fs.createWriteStream('./test.txt');

    req.on('end', function() {
        res.send(200);
    });
    req.pipe(fileStream);
});

app.listen(3000);

test.html

<html>
    <body>
        <script type="text/javascript">

var xhr = new XMLHttpRequest();
xhr.onreadystatechange = function() {
    if (xhr.readyState == 4)
        alert('XMLHttpRequest POST request performed.');
};

xhr.open("POST", '/test', true);
xhr.send('12345');

        </script>
        <a href="/test.html">Reload</a> (This will only work every second click)
    </body>
</html>

Gabriel

@bnoordhuis

Thanks for the report. Your test case depends on express however and we don't support third-party modules. I'll have to ask you to either rewrite it or contact the express people.

@bnoordhuis bnoordhuis closed this
@gabipetrovay

Hi Ben,

Thanks for the short reply, but I don't agree with you here since it looks like you stopped parsing at line 1. Express was used as a way to make the example cleaner. And I guess it's pretty obvious the problem has probably nothing to do with express. Neither do have: "XMLHttpRequest", "pipe", "writable file stream", "request".

But because of that probably, I went back and minimized it further for you:

http.createServer(function (req, res) {
  console.log(req.url);
  if (req.url == '/test') {
    var fs = require('fs');
    var fileStream = fs.createWriteStream('./test.txt');

    req.on('end', function() {
        res.writeHead(200);
        res.end();
    });
    req.pipe(fileStream);
  } else {
    var response = '<html><body><script type="text/javascript">var xhr = new XMLHttpRequest(); xhr.onreadystatechange = function() { if (xhr.readyState == 4) alert("XMLHttpRequest POST request performed."); }; xhr.open("POST", "/test", true); xhr.send("12345");</script><a href="/test.html">Reload</a> (This will only work every second click)</body></html>'
    res.end(response);
  }
}).listen(3000);
console.log('Server running at http://127.0.0.1:3000/');
@gabipetrovay

"You don't have permission to reopen this issue."

Do you mind reopening it?

Thanks!

@bnoordhuis bnoordhuis reopened this
@koichik
Owner

@gabipetrovay: Thanks, I confirmed it.
This problem occurs by the following steps:

  • http.ServerRequest emits 'data' event.
  • pipe() calls fs.WriteStream.write(), it always returns false.
  • pipe() calls http.ServerRequest.pause(), and it calls net.Socket.pause().
  • However, because the request body has been already received, http.ServerRequest emits 'end' event (see also #1040).
  • pipe() calls fs.WriteStream.end(), and destroys the pipeline.

Therefore, because nobody calls http.ServerRequest.resume(), net.Socket is still paused. This is the reason why the second request to use Connection: Keep-Alive is not handled.
So, we should resume the socket.

@koichik
Owner

Can someone review koichik/node@b4da500?

@koichik
Owner
@bnoordhuis

@koichik: LGTM. I ran some http_simple benchmarks and didn't notice a performance difference (nothing above the level of noise anyway) but can you double-check that before you merge it?

@koichik
Owner

@bnoordhuis Thanks!

can you double-check that before you merge it?

I confirmed it. I did not find the difference, too. Merging.

@koichik koichik closed this in a337ac7
@koichik
Owner

@gabipetrovay Thanks for the report, fixed in a337ac7.

@isaacs isaacs referenced this issue from a commit
Commit has since been removed from the repository and is no longer available.
@isaacs isaacs referenced this issue from a commit in isaacs/node
@isaacs isaacs 2012.01.06, Version 0.6.7 (stable)
* Upgrade V8 to 3.6.6.15

* Upgrade npm to 1.1.0-beta-10 (isaacs)

* many doc updates (Ben Noordhuis, Jeremy Martin, koichik, Dave Irvine,
  Seong-Rak Choi, Shannen, Adam Malcontenti-Wilson, koichik)

* #2438 segfault in node v0.6.6

* dgram, timers: fix memory leaks (Ben Noordhuis, Yoshihiro Kukuchi)

* repl: fix repl.start not passing the `ignoreUndefined` arg (Damon Oehlman)

* #1980: Socket.pause null reference when called on a closed Stream (koichik)

* #2263: XMLHttpRequest piped in a writable file stream hang (koichik)

* #2069: http resource leak (koichik)

* buffer.readInt global pollution fix (Phil Sung)

* timers: fix performance regression (Ben Noordhuis)

* #2308, #2246: node swallows openssl error on request (koichik)

* #2114: timers: remove _idleTimeout from item in .unenroll() (James Hartig)

* #2379: debugger: Request backtrace w/o refs (Fedor Indutny)

* simple DTrace ustack helper (Dave Pacheco)

* crypto: rewrite HexDecode without snprintf (Roman Shtylman)

* crypto: add SecureContext.clearOptions() method (Ben Noordhuis)

* crypto: don't ignore DH init errors (Ben Noordhuis)
f15ef99
@isaacs isaacs referenced this issue from a commit
@isaacs isaacs 2012.01.06, Version 0.6.7 (stable)
* V8 hash collision fix (Breaks MIPS) (Bert Belder, Erik Corry)

* Upgrade V8 to 3.6.6.15

* Upgrade npm to 1.1.0-beta-10 (isaacs)

* many doc updates (Ben Noordhuis, Jeremy Martin, koichik, Dave Irvine,
  Seong-Rak Choi, Shannen, Adam Malcontenti-Wilson, koichik)

* Fix segfault in node_http_parser.cc

* dgram, timers: fix memory leaks (Ben Noordhuis, Yoshihiro Kukuchi)

* repl: fix repl.start not passing the `ignoreUndefined` arg (Damon Oehlman)

* #1980: Socket.pause null reference when called on a closed Stream (koichik)

* #2263: XMLHttpRequest piped in a writable file stream hang (koichik)

* #2069: http resource leak (koichik)

* buffer.readInt global pollution fix (Phil Sung)

* timers: fix performance regression (Ben Noordhuis)

* #2308, #2246: node swallows openssl error on request (koichik)

* #2114: timers: remove _idleTimeout from item in .unenroll() (James Hartig)

* #2379: debugger: Request backtrace w/o refs (Fedor Indutny)

* simple DTrace ustack helper (Dave Pacheco)

* crypto: rewrite HexDecode without snprintf (Roman Shtylman)

* crypto: don't ignore DH init errors (Ben Noordhuis)
d5a189a
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.