Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add generator iteration support #207

Closed
juliangruber opened this issue Feb 3, 2014 · 28 comments
Closed

add generator iteration support #207

juliangruber opened this issue Feb 3, 2014 · 28 comments

Comments

@juliangruber
Copy link
Contributor

Currently there are only two hacky ways to create streaming response bodies:

Streams and callbacks:

app.use(function*(){
  var stream = PassThrough();
  this.body(stream);

  (function next(){
    queue.next(function(err, data){
      if (err) return stream.emit('error', err);
      if (!stream.writable) return; // response ended
      stream.push(data);
      if (!stream.writable) return; // response ended
      next();
    });
  })();
});

Streams and co:

app.use(function*(){
  var stream = PassThrough();
  this.body(stream);

  co(function*(){
    while (true) {
      var data = yield queue.next();
      if (!stream.writable) return; // response ended
      stream.push(data);
      if (!stream.writable) return; // response ended
    }
  })(this.onerror);
});

Both are extremely koa averse and the 2nd one is slow. We need an ideomatic koa way to create streaming response bodies.

For reference: Just iterating over gens doesn't work as the headers won't be sent. Example:

app.use(function*(){
  this.body = PassThrough();
  while (true) {
    if (!this.body.writable) return; // response ended
    this.body.push(yield queue.next());
  }
});
@juliangruber
Copy link
Contributor Author

Lame idea: converting generators to streams. Problem: How to model data processing while staying with js methods of flow control?

app.use(function*(){
  this.body = toStream(queue.next);
  this.body = toStream(queue.next, transform);
  // transform could be sync fn, async fn, gen fn or stream
});

@juliangruber
Copy link
Contributor Author

Less lame idea: Send headers when the response is first written to:

app.use(function*(){
  while (true) {
    if (!this.response.write(yield queue.next())) return;
  }
});

@juliangruber
Copy link
Contributor Author

maybe even less of a lame idea: set an iterator as body:

app.use(function*(){
  this.body = queue.next;
});

@juliangruber
Copy link
Contributor Author

@visionmedia said that with iterators streaming data should always be pulled instead of pushed, so you have backpressure. That would work with a generator function being the body, and koa pulling from it.

@tj
Copy link
Member

tj commented Feb 3, 2014

yea that'd be cool, only lame thing then I guess is that we have node streams, and then koa/co-ish streams

@karlbohlmark
Copy link
Contributor

I think it's very reasonable. Also it is just like they do it in python: http://en.wikipedia.org/wiki/Web_Server_Gateway_Interface

@tj
Copy link
Member

tj commented Feb 3, 2014

yeah they do it in ruby/rack as well, I'd just like to have a minimal number of things that can be a .body if possible. It would be nice to either have this and require that streams be wrapped, or the other way around

@juliangruber
Copy link
Contributor Author

We can do that with co-read:

var read = require('co-read');

app.use(function*(){
  this.body = read(createStream());
});

@jonathanong
Copy link
Member

@visionmedia is this something you want to support? i prefer not to support anything unless there's an official spec or something like the new streams

@juliangruber
Copy link
Contributor Author

We do need some kind of solution eventually, I recently started disliking co-streams because the hide flow control, I'd like to write something like this:

var read = require('co-read');
var write = require('co-write');

app.use(function*(){
  var con = net.connect(3000);
  var data;
  while (data = yield read(con)) yield write(res, data);
});
  • flow control is still explicit
  • backpressure is handled (yield write(res))
  • sockets are cleaned up (yield write(res) throws when the request ended, need domains for sockets to close then)

@jonathanong
Copy link
Member

that won't even work though. you have put it in a separate co

my only issue is that we'll end up supporting more body types, and any upstream middleware would have to support these too. it's easier just to convert it to a stream.

@jonathanong
Copy link
Member

imo if we add a body type, it should only be https://github.com/whatwg/streams

@juliangruber
Copy link
Contributor Author

It would work if we patched res.write and res.end to detect early writes and then sent out the headers.

@jonathanong
Copy link
Member

oh lol but then you're bypassing koa's entire middleware system. easier just to make your own framework or something. don't think you need koa for this :P

@juliangruber
Copy link
Contributor Author

yeah I think this is odd with koa right now because koa is going the descriptive route and hiding lots of the explicit calls you'd have to make. With node streams or co streams you stay descriptive still, but those read/write calls are the oposite direction again.

@jonathanong
Copy link
Member

yeah. would better as a lib that does:

this.body = pipe(net.connect(3000), this);

@jonathanong
Copy link
Member

i think most of these things should be separate libraries anyways because you can't expect users to do error handling correctly or you don't want to repeat yourself every time you use it

@tj
Copy link
Member

tj commented Feb 26, 2014

dear god, whatwg/streams looks possibly worse than what we have now :s haha.. oh man.. javascript

@jonathanong
Copy link
Member

Lol at least you can yield it though. Better than all this crazy emitter handling

@juliangruber
Copy link
Contributor Author

whatwg/streams uses promises, 'nuf said

@jonathanong
Copy link
Member

@juliangruber mind if we close this for now?

@juliangruber
Copy link
Contributor Author

What i currently have to do is this:

var PassThrough = require('stream').PassThrough;
var write = require('koa-write');
var co = require('co');

app.use(function*(){
  this.body = PassThrough();

  co(function*(){
    while (true) {
      yield write(this, yield read());
    }
  }).call(this, this.onerror);
});

That's way too much boilerplate and too much to do wrong. Creating a co inside another co seems like the uber antipattern.

I need koa-write because if you just write to the passthrough using co-write it doesn't throw when the request is already closed, as it does when writing to a response object directly (it checks stream.socket.writable).

How I'd like this piece of code to look ~ like:

app.use(function*(){
  this.stream();
  while (true) {
    yield this.write(yield read());
  }
});

So nope def. don't close this :D

@jonathanong
Copy link
Member

i don't know man. i like the first one better. i'm assuming this.stream() sends the header and stuff, but i'm -1 on that since now we have to support sending headers in two different places. i also don't like how you've made the middleware blocking since it messes up the upstream/downstream stuff.

why not just do

app.use(function* () {
  this.body = createPassThroughFrom(read)
})

i don't see the need to make everything super generator-convenient. this is what koa would be doing anyways. with #255 the errors would be handled as well.

@juliangruber
Copy link
Contributor Author

Yeah that kinda looks more koa-y too, describing flow instead of explicitly carrying it out. I'm not sure about the direction we should go here, this issue made me not use koa for a project though, way simpler to write streaming reponses with http.createServer.

@jonathanong
Copy link
Member

meh. if you don't need to use a framework, don't use a framework. i think that this stuff should always go into a lib/module anyways, so you should always create that createPassThroughFrom() stream. then using koa or just http.createServer() would basically be the same. this.body = vs. stream.pipe(res).

@tj
Copy link
Member

tj commented Apr 10, 2014

it would definitely be nice if streams weren't the odd man out, I know Bert wants to make them more generator-friendly for the next version of node but yeah who knows about that haha

@jonathanong
Copy link
Member

@juliangruber i'm going to close this. i don't think we should be adding more types unless it's either a standard like es7 streams or a de facto standard like node streams.

@zackify
Copy link

zackify commented Dec 4, 2015

Any ideas on how to do this best in koa 2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants