http 'body' event

[Jeremy Apthorp]

Hi list,

I was reading through the node API this afternoon, and came across
something that struck me as odd.

There's a race condition in handling HTTP body data. This example is
given in the documentation:

var google = node.http.createClient(80, "google.com");
var request = google.get("/");
request.finish(function (response) {
puts("STATUS: " + response.statusCode);
puts("HEADERS: " + JSON.stringify(response.headers));
response.setBodyEncoding("utf8");
response.addListener("body", function (chunk) {
puts("BODY: " + chunk);
});
});

The race condition is that, if enough time passes before the
response.addListener function gets called, the response object emits
the 'body' event when there are no listeners attached to it. This
modification illustrates the issue:

var google = node.http.createClient(80, 'google.com');
var request = google.get('/');
request.finish(function (resp) {
puts("STATUS: " + resp.statusCode);
puts("HEADERS: " +JSON.stringify(resp.headers));
setTimeout(function () {
resp.setBodyEncoding('utf8');
resp.addListener('body', function(chunk) {
puts("BODY: " + chunk);
});
}, 1000);
});

The 'body' handler never gets called.

You could put the addListener call at the very beginning of the
.finish callback function, but the race is still there...

The immediately obvious solution to this problem would be to have node
buffer the body chunks until resp.receiveBody() is called, at which
point it will emit all the body chunk events it's received. That feels
like a bit of a hack (what happens if that function never gets
called?) but it'd stop my hackles standing up every time I use that
part of the API.

Jeremy

[Hagen]

There's a race condition in handling HTTP body data. This example is
given in the documentation:

As far as I can tell, there is no race condition. Node assumes the setup to be complete, once finish() exists. In your "race condition" example, you are violating that assumption (remember it's all async, so finish() is long gone when the timeout comes back)

[Jeremy Apthorp]

Aha, the trigger at the end of finish() was the piece of information I
was missing. It would be nice if that was documented a bit more
clearly.

Thanks,
Jeremy



2009/9/16 Hagen <six...@gmail.com>:

[ryan dahl]

On Wed, Sep 16, 2009 at 10:17 AM, Jeremy Apthorp <norn...@gmail.com> wrote:
>
> Aha, the trigger at the end of finish() was the piece of information I
> was missing. It would be nice if that was documented a bit more
> clearly.

The body event will not be called until after the "response" callback
(the one passed to finish()) is done. If you delay in setting a
listener, you will simply miss part of the body.

Yes, the documentation needs work. What would be a good way to explain this?

[Jeremy Apthorp]

Something like 'the response object does not emit any events until the
finish() function has returned. All listeners on the response object
should be added from within the function passed to finish().'

Would be good to put that both under the example given and in the
description of finish().

j



2009/9/16 ryan dahl <coldre...@gmail.com>:

[Jacob Rus]

> Something like 'the response object does not emit any events until the
> finish() function has returned. All listeners on the response object
> should be added from within the function passed to finish().'

Why should all listeners be added from within the function passed to
finish? I don't think that's even generally the proper approach, let
alone universally.

--Jacob

[Jeremy Apthorp]

What is the proper approach?

j



2009/9/17 Jacob Rus <jaco...@gmail.com>:

[Jacob Rus]

> What is the proper approach?

Oh, sorry, I was confusing myself. Given that the inside of the
`finish` function is the first place we get access to the response
object after it is created, it probably is usually the proper place to
attach callbacks. Nevermind. –Jacob

[ryan dahl]

On Thu, Sep 17, 2009 at 2:01 AM, Jeremy Apthorp <norn...@gmail.com> wrote:
>
> Something like 'the response object does not emit any events until the
> finish() function has returned. All listeners on the response object
> should be added from within the function passed to finish().'
>
> Would be good to put that both under the example given and in the
> description of finish().

Thanks. More explanation added in 6f31a37.

[JaapAap]

Hello there and thanks for very interesting tool.

I was wondering how this problem is solved on the server side; I am
trying to save a multipart formdata body to disk:

node.http.createServer(function (req, res) {
var chunks = 0
node.fs.open("data.txt", node.O_WRONLY | node.O_CREAT,
0666).addCallback(function(file) {
puts('created file');
req.addListener('body',function(chunk) {
chunks += 1;
puts('got a chunk');
node.fs.write(file, chunk, null).addCallback(function() {
puts('wrote a chunk');
});
});
puts('added chunk listener');
req.addListener('complete',function() {
puts('body is complete; number of chunks: '+chunks);
node.fs.close(file).addCallback(function() {
puts('closed file');
});
});
puts('added close listener');
});
node.fs.cat("page.html").addCallback(function (content) {
res.sendHeader(200, {"Content-Type": "text/html"});
res.sendBody(content);
res.finish();
});
}).listen(8000);


I try to wrap the adding of the eventlisteners in req.pause() -
req.resume(), but that didn't help. Am I completely missing the point
here?

Thanks

[Hagen]

You haven't said, what the actual problem is, but

   node.fs.cat("page.html").addCallback(function (content) {
       res.sendHeader(200, {"Content-Type": "text/html"});
       res.sendBody(content);
       res.finish();
   });

this code is executed independent of the actual receive, i.e. you will return data before ensuring that you actually got the complete request data. Move the block into req.addListener('complete',function() {}).

Cheers,

Hagen

[Kees Varekamp]

Sorry about that: The problem was that I miss the first bunch of chunks. So it looks like they are being sent before I have registered the 'body' event listener. Is there a way to fix that? req.pause() seems not to help. My new code is:

node.http.createServer(function (req, res) {
    var chunks = 0

    req.pause();
    node.fs.open("data.txt", node.O_WRONLY | node.O_CREAT | node.O_TRUNC, 0666).addCallback(function(file) {

        puts('created file');
        req.addListener('body',function(chunk) {
            chunks += 1;
            puts('got a chunk');
            node.fs.write(file, chunk, null).addCallback(function() {
                puts('wrote a chunk');
            });
        });
        puts('added chunk listener');
        req.addListener('complete',function() {
            puts('body is complete; number of chunks: '+chunks);

            node.fs.cat("page.html").addCallback(function (content) {
                puts('sending reply');

                res.sendHeader(200, {"Content-Type": "text/html"});
                res.sendBody(content);
                res.finish();
            });

            node.fs.close(file).addCallback(function() {
                puts('closed file');
            });
        });
        puts('added close listener');

        req.resume();
    });
}).listen(8000);

This one only replies when there is a large body. Which makes sense, because the 'complete' listener is never called if the complete event fires before the listener is registered. BTW: I don't think the response needs to wait for the request fo finish anyway in this case since it is independent of the request.

Thanks

[Hagen]

All listeners MUST be registered before the function passed to createServer() returns. Yours doesn't, as they are registered in another callback. Hence, you are missing all callbacks until the fle has been opened.

--
Dissertations are a successful walk through a minefield -- summarizing them is not. - Roy Fielding

[Kees Varekamp]

OK, that makes sense, thanks. But now I'm still lost: How do I get a file handle without using a callback?

[Hagen]

You don't. You will need to buffer the body callbacks you receive until you have the file handle.

something like this:

var buffer = [];

var file;

req.addListener('body',function(chunk) {

  buffer.push(chunk);

  if (file) {

    var buffered;

    while(buffered = bodyBuffer.shift()) {

      node.fs.write(file, buffered, null).addCallback(function() {

        puts('wrote a chunk');

      });

    }

  }

});

don't forget to check the buffer in "complete" as well.

[Ryan Dahl]

On Sun, Sep 27, 2009 at 2:37 PM, Kees Varekamp <kees.v...@gmail.com> wrote:
> OK, that makes sense, thanks. But now I'm still lost: How do I get a file
> handle without using a callback?

So, opening a file (node.fs.open()) doesn't take zero amount of time.
Node might actually have to spin the disk. That means, if you want to
call node.fs.open() on each request, but the HTTP client doesn't wait
for that to happen - they are sending the body now. So you're going to
have to buffer the body until the file is open, and once it's open you
can train the buffer to disk. This sounds hard, but you can be assured
that only one callback is happening at a time, so you don't need to
worry about interfering with other callbacks.

I'd probably use the buffered file object, which is undocumented.
(Sorry, that will be corrected soon.)

include("/file.js");
server = node.http.createServer(function (req, res) {
var f = new File("file.txt", "w+");
req.addListener("body", function (chunk) {
f.write(chunk);
});
req.addListener("complete", function () {
f.close();
});
});

[Kees Varekamp]

Thanks both, I think I actually understand it now :-)