why doesn't this use .pipe()? #116

Closed
dominictarr opened this Issue Sep 22, 2011 · 17 comments

Projects

None yet

7 participants

@dominictarr
Contributor

this would be way simpler if we used pipe

something like this:

createServer(function (req, res) {

  var _req =  http.request({...}) //or whatever

  req.pipe(_req)

  _req.on('response', function (_res) {
    res.writeHead(_res.statusCode, _res.headers)
    _res.pipe(res)
  })
  _req.on('error',...) //send back a 503 or something?
})

the code we currently have just duplicates functionality in pipe

@indexzero
Member

Pipe is not ready for this kind of operation. @mikeal could probably speak to why that is

@indexzero indexzero closed this Sep 22, 2011
@mikeal
mikeal commented Sep 22, 2011

by "this" do you mean the whole project?

i don't see why you couldn't use pipe().

in the case where you want to mutate the data you can just pipe() to a stream that handles your mutation api.

in the case where you need to buffer you can use BufferedStream until streams2 gets merged (which includes the .buffer()) method.

but yes, i doubt it would be a trivial patch.

@indexzero
Member

The problem is really with the optimism of the current pipe implementation:

https://github.com/joyent/node/blob/master/lib/stream.js

There are no guards (i.e. try/catch) around calls to .write() (which can throw). Also the source.readable around stream.resume() are apparently not enough:
https://github.com/joyent/node/blob/master/lib/stream.js#L127
https://github.com/joyent/node/blob/master/lib/stream.js#L34-38

Recent commits have been added by @isaacs to fix problems regarding calls to those two methods throwing uncaughtException events:

7bda25b
558a8a4

If we can solve these problems within the existing .pipe() implementation then it is probably fine, but I suspect it won't work because there are a couple of special cases for HTTP streams that we handle in node-http-proxy:

https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy/http-proxy.js#L214-218

In short, I think .pipe() is awesome and when you have a little information around the HTTP stream you are trying to pipe, it works like a charm. As a generic silver bullet though, it still has a little way to go.

@mikeal
mikeal commented Sep 22, 2011

When does write() throw? I believe you that it does but in what case?

I mean, this is really slow. This many guards is just really slow.

And when does resume() throw? jesus!

These sound like core bugs that we're catching and I just want to make sure they are fixed in core.

@mikeal
mikeal commented Sep 22, 2011

Also, core pipe() will never have this many guards cause it'll totally fuck our benchmarks.

@mikeal
mikeal commented Sep 22, 2011

Also, I really really really can't wait for domains, which would make this all a lot simpler to catch.

@indexzero
Member

When I spoke with @isaacs about finding these throwing cases he said they were very difficult to reproduce which makes sense to me given the brow-beating both Nodejitsu and Joyent put node-http-proxy thru in production.

I agree with you that the .pipe() method in core shouldn't have this many guards for performance reasons, and that misalignment of goals is probably why .pipe() isn't appropriate here. Keeping the process alive is paramount to avoid dropped WebSocket connections.

@isaacs
Contributor
isaacs commented Sep 22, 2011

There are bugs in core where .write() or .resume() on an http response stream will throw if the socket is closed.

I would like to solve this by 0.6. If for some reason it turns out it's not possible to fix by 0.6 because it requires too much API change, then we need to know why, lay the appropriate deprecations, and fix it asap in 0.7. But the first step is figure out what that API needs to be, and how far we are from it.

@dominictarr
Contributor

https://github.com/nodejitsu/node-http-proxy/blob/master/lib/node-http-proxy/http-proxy.js#L214-218

sounds like a core issue too, I mean, it should a bodyless response should still emit 'end'

@mikeal
mikeal commented Sep 27, 2011

so, the write() and end() throwing issues, insuring "end" is emitted, and removing close() in favor of destroy() have all made it in to the spec in streams2 and are part of the discussion in that pull request. i also have buffering in there as well. we can back burner this discussion until 0.6.0 is released.

@indexzero indexzero reopened this Sep 27, 2011
@dominictarr
Contributor

In some work I was recently doing with streams, I was getting this error, (was piping tar -c to tar -xz, which crashes tar), however I was able to keep the process up by listening on 'uncaughtException'.

I was piping from an incoming tar file to disk via http, and though the write to disk socket was breaking, since I was managing to catch it, I was still able to respond to the http request with an error message.

I don't see uncaughtException anywhere in node-http-proxy, maybe we could use that and then we might not need the guards?

@coderarity
Contributor

@isaacs with 0.6.0 out would it make sense to look at this again?

@indexzero
Member

From what I've heard from @isaacs this doesn't make sense until node@0.10.0 has a stable domains API

@indexzero indexzero closed this Jul 22, 2012
@mmalecki
Contributor

How are domains involved here? I'm down with maintaining our code to make it match current API.

Also, looks like .write() shouldn't throw anymore. @isaacs, is that correct?

@abarre
abarre commented Apr 24, 2013

Since 0.10 is released, can we consider this refactoring again?

I see that @mmalecki has prepared a branch https://github.com/nodejitsu/node-http-proxy/tree/refactor with the pipe implementation a few month ago.

The code would be far easier to maintain and also it would simplify a lot the modification of the HTML response going back to the user.

@indexzero
Member

There is work on this to rewrite most of this as a stream, if you'd like to contribute it would be greatly appreciated. See: https://github.com/nodejitsu/node-http-proxy/tree/streams2

@abarre
abarre commented May 2, 2013

ok, thank you for the reply, I will definitely have a look at this branch. Is there already an issue tracking the changes ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment