Browse files

[fix] Collect stream data before nextTick, Fixes #81

  • Loading branch information...
1 parent 0e62b87 commit c8bf2b83c3431c7327d23b22a6d6a49eb61ae1c6 @pksunkara pksunkara committed Mar 23, 2012
Showing with 12 additions and 0 deletions.
  1. +5 −0 examples/http.js
  2. +7 −0 lib/director/http/index.js
View
5 examples/http.js
@@ -17,5 +17,10 @@ router.get(/foo/, function () {
this.res.end('hello world\n');
});
+router.post(/foo/, function () {
+ this.res.writeHead(200, { 'Content-Type': 'application/json' })
+ this.res.end(JSON.stringify(this.req.body));
+});
+
server.listen(8080);
console.log('vanilla http server with director running on 8080');
View
7 lib/director/http/index.js
@@ -122,6 +122,13 @@ Router.prototype.dispatch = function (req, res, callback) {
}
if (!stream) {
+
+ req.chunks = [];
+
+ req.on('data', function (data) {
+ req.chunks.push(data.toString());
+ });
+
process.nextTick(function () {
//
// If there is no streaming required on any of the functions on the

18 comments on commit c8bf2b8

@indexzero
a decoupled application framework member

This looks wrong to me.

@pksunkara

How so?

@indexzero
a decoupled application framework member

If req.chunks is defined you are overriding it. It will be defined when working with union, so this breaks all union-based buffering and streaming.

@pksunkara

That is why options.stream is there.

If you are using with union, you should use options.stream

@pksunkara

@indexzero Are your concerns allayed?

@twilson63

This does appear to cause buffered flatiron/union http posts to create duplicate data in body.

 var director, router, server, union;

union = require('union');

director = require('director');

router = new director.http.Router();

server = union.createServer({
  before: [
    function(req, res) {
      var found;
      found = router.dispatch(req, res);
      if (!found) return res.emit('next');
    }
  ]
});

router.post('/', function() {
  console.log(this.req.body);
  this.res.end('hello');
});

server.listen(3000);

when client sends

curl -XPOST -d 'title=foo' http://localhost:3000

outputs

{ title: 'footitle=foo' }

Is it possible to check if union is serving director before adding listener?

@pksunkara

@twilson63

With the latest master in director.

 var director, router, server, union;

union = require('union');

director = require('director');

router = new director.http.Router().configure({ parseBody: false });

server = union.createServer({
  before: [
    function(req, res) {
      var found;
      found = router.dispatch(req, res);
      if (!found) return res.emit('next');
    }
  ]
});

router.post('/', function() {
  console.log(this.req.body);
  this.res.end('hello');
});

server.listen(3000);

when client sends

curl -XPOST -d 'title=foo' http://localhost:3000

outputs

{ title: 'foo' }
@twilson63

Awesome, so

router = new director.http.Router().configure({ parseBody: false });

So will this flag be the default in flatiron, or will it have to be set for every new flatiron project?

Thanks

@pksunkara

It is going to be default from next version. Though it still have to be committed.

@jfhbrook

So, like, nobody thought to ask why we have two body parsers? Maybe one of them should just be removed?

@jfhbrook

Like, I don't think it's the router's job to be doing this body parsing stuff necessarily.

@pksunkara

@jesusabdullah Some people only wanted to use director and are complaining that director doesn't parse the body. Individually both of them should parse the body because that is what flatiron is about. It's about being unobtrusive. But when they are combined in flatiron, director is configured not to parse.

@jfhbrook

No, I disagree.

First of all, only one of the projects should be concerned with buffering and parsing the body. I think it makes the most sense to be in union.

Second, this project should degrade gracefully if one isn't parsing the body with union, and I think that means just that you'll have to parse the body yourself. I think any user that's using the base http object with director isn't going to want this in here. It's this concept of body parsing that shouldn't intrude on the router.

@pksunkara

That was your comment, #81 (comment) and you asked me to do it in #81 (comment)

@jfhbrook

I didn't ask for code duplication, I asked for director to work.

@mmalecki

I agree with @jesusabdullah. If people want to do buffering and parsing, they should use union. director is a router. We should keep it minimal and protocol-agnostic.

@pksunkara

I agree too. Therefore we should remove all the buffering and parsing from director. https://github.com/flatiron/director/blob/master/lib/director/http/index.js#L112-L203.

@indexzero You have written it. What do you decide?

@indexzero
a decoupled application framework member

I have strong opinions about this. That's why I wrote it. Dont touch any code. I will resolve it and reach out.

Please sign in to comment.