mafintosh / pump

pipe streams together and close all of them if one of them closes
MIT License
907 stars 59 forks source link

Question: when not to use `pump`? #52

Open DenisFrezzato opened 4 years ago

DenisFrezzato commented 4 years ago

Consider this example.

As soon as stream data have been collected, the HTTP connection is closed just before someAsyncStuff is invoked. Refactoring pump(req, busboy... to req.pipe(busboy) fixes the issue. It's also my understanding that using pump is a best practice for piping streams (for error handling and some clean-up), but in this case it introduces an issue. I would like to understand why this example is not working as I was expecting.

import * as Busboy from 'busboy'
import * as express from 'express'
import * as pump from 'pump'
import concat = require('concat-stream')

const app = express()

app.post('/', (req, res) => {
  const busboy = new Busboy({ headers: req.headers })

  busboy.on('file', (_, file) => {
    pump(
      file,
      concat(buffer =>
        someAsyncStuff(buffer.toString())
          .then(length => res.send({ length }))
          .catch(err => res.status(500).send(err.message))
      ),
      err => {
        if (err) res.status(500).send(err.message)
      }
    )
  })

  pump(req, busboy, err => {
    if (err) res.status(500).send(err.message)
  })
})

function someAsyncStuff(s: string): Promise<number> {
  return new Promise(resolve => setTimeout(() => resolve(s.length), 1))
}

app.listen('3000')
mafintosh commented 4 years ago

which error handler is being triggered and what's the error?

DenisFrezzato commented 4 years ago

No error at all. The HTTP connection is just closed, but someAsyncStuff goes on with its execution.

mafintosh commented 4 years ago

pump doesn't do anything magic, don't know enough about the modules used to know if they do. If you share a runnable end-to-end gist, I'm happy to try it.

DenisFrezzato commented 4 years ago

Do you have any playground to suggest with which I can make this example with Express work? The code I posted is a complete example, you could just copy-paste it in a .ts file and run it and try a POST request with some file in the payload.

mafintosh commented 4 years ago

A gist with a thing I can run with npm install && node server.js and a curl command that reproduces the issue you are seeing (or something other than curl).

DenisFrezzato commented 4 years ago

Here it is https://github.com/DenisFrezzato/question-pump

EDIT: I also tried with pipeline and the result is the same (but I guess this is not surprising).