dominictarr / mux-demux

mutiplex-demultiplex multiple streams through a single text Stream
MIT License
179 stars 15 forks source link

only write on .pipe #23

Closed juliangruber closed 11 years ago

juliangruber commented 11 years ago

Me and @ralphtheninja had this problem that MuxDemux wrote its meta infos before mux-demux was piped anywhere, like:

// Server
var mdm = require('mux-demux')()
net.createServer(function (c) {
  c.pipe(mdm).pipe(c)
}).listen(3000)

// Client
var mdm = require('mux-demux')()
mdm.pipe(net.connect(3000)).pipe(mdm)

If you inspect the traffic you see that the server-mdm wrote its meta before the connection to the client was up. What I saw you doing to avoid this is you create the mdm object inside the net.createServer-handler.

That works for this case, but libraries using mux-demux (like multilevel) have the problem that they can't expose a simple stream interface anymore.

// Breaks
var server = multilevel.server('/db')
net.createServer(function (c) {
  c.pipe(server).pipe(c)
})

// Works
net.createServer(function (c) {
  c.pipe(multilevel.server('/db')).pipe(c)
})

multilevel directly exports its underlying MuxDemux-instance. What I could do is wrap that in a paused duplex-stream and listen for mdm.on('pipe', fn) but I think that this should happen inside mux-demux, in order not to leak its implementation details.

If you want I can hack up a PR that for every pipe event emits its stream meta.

dominictarr commented 11 years ago

You should create a mux-demux immediately before you pipe it, or explicitly pause it.

This will work:

var server = multilevel.server('/db').pause()
net.createServer(function (c) {
  c.pipe(server.resume()).pipe(c)
})
ralphtheninja commented 11 years ago

Just tried it in https://github.com/juliangruber/multilevel/blob/master/test/util.js

This doesn't work:

    var server = multilevel.server(__dirname + '/db').pause()
    net.createServer(function (con) {
      con.pipe(server.resume()).pipe(con)
    }).listen(port)
dominictarr commented 11 years ago

oh, oops

just realized, resume() resumes immediately, so you need to

var server = multilevel.server(__dirname + '/db').pause()
    net.createServer(function (con) {
      con.pipe(server).pipe(con)
      server.resume()
    }).listen(port)

sorry.

returning it prepaused is unusual. If really want to do that make sure you document it CLEARLY.

Generally, you shouldn't need to create a stream before you pipe it, so this is best:

net.createServer(function (con) {
   con.pipe(multilevel.server(__dirname + '/db')).pipe(con)
}).listen(port)

actually, this is wrong:

var server = multilevel.server(__dirname + '/db').pause()
    net.createServer(function (con) {
      con.pipe(server).pipe(con)
      server.resume()
    }).listen(port)

because when multiple connections join, they will all be piped onto the same stream. instead, you want a stream from multilevel for each connection.

Also, can you alias multilevel.server and multilevel.client to multilevel.createServerStream and multilevel.createClientStream as it will make their behaviour more obvious?

juliangruber commented 11 years ago

ok. @substack I think that "you shouldn't need to create a stream before you pipe it" pattern should go in the stream handbook?