ArtskydJ / node-sox-stream

:mega: A stream-friendly wrapper around SoX
53 stars 14 forks source link

Failure causes infinite loop #4

Closed MGlauser closed 8 years ago

MGlauser commented 8 years ago

This line is causing an infinite loop. The problem comes when a failure condition is created for example when being sent an mp3 file and telling sox it’s a wav file. (Aside from that being a bad thing, the module must handle it gracefully.)

Sox throws an error as it should.
The tmpFile does not exist so cleanup throws an error and you’re in an infinite loop.

function emitErr(err) {
    tmpFile.cleanup( duplex.emit.bind(duplex, 'error', err) )
}

The suggested way to deal with it is in this style, but forgive me for not knowing how to take the above line and implement it like the below line.

function emitErr(err) { if (err.code !== 'ENOENT') { // Avoid infinite loops tmpFile.cleanup(duplex.emit.bind(duplex, 'error', err)); } }

ArtskydJ commented 8 years ago

Whoops, good catch!

I'll try to make a repeatable test case at some point...

MGlauser commented 8 years ago

You’re a good man.

BTW: I did successfully test this with: sox-stream/index.js:29

function emitErr(err) { if (err.code !== 'ENOENT') { // Avoid infinite loops tmpFile.cleanup(duplex.emit.bind(duplex, 'error', err)) } }

And a different but related question:

Scenario: using sox-stream and sox fails to transcode, how do I send a 500 http status code to the client? Any attempt to send a 500 results in the dreaded ‘headers already sent’ error. So my failure case client gets a 200 with an empty result.

I’ve tried lazypipe() and other such interrupted or conditional methods. Since you have so much experience with streams (and this being my 1st dive into it with NodeJS)

On May 5, 2016, at 10:07 AM, Joseph Dykstra notifications@github.com wrote:

Whoops, good catch!

I'll try to make a repeatable test case at some point...

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/ArtskydJ/sox-stream/issues/4#issuecomment-217212349

ArtskydJ commented 8 years ago

If you pipe the output of sox-stream to your response, then if an error occurs during the transcoding process, then you will have already responded with part of the file.

As far as I know, the headers are sent at the beginning of the response, so you can't start sending the file until it is finished transcoding.

So you need to buffer the file to memory or disk:

Memory version, concat-stream or simple-concat

var concat = require('simple-concat')

// ...
stream.pipe(convert).pipe(concat(function (err, buf) {
    if (err) {
        res.writeHead(500)
        res.end()
    } else {
        res.writeHead(200)
        res.end(buf)
    }
}))

Disk version:

stream.pipe(fs.createWriteStream('herp_derp'))
    .on('end', function () { // Or is it 'close'?
        res.writeHead(200)
        fs.createReadStream('herp_derp').pipe(res)
    }).on('error', function (err) {
        res.writeHead(500)
        res.end()
    })

If you use disk caching, maybe sox.js will work for you?

MGlauser commented 8 years ago

Thank you for this!

On May 5, 2016, at 11:02 AM, Joseph Dykstra notifications@github.com wrote:

If you pipe the output of sox-stream to your response, then if an error occurs during the transcoding process, then you will have already responded with part of the file.

As far as I know, the headers are sent at the beginning of the response. So you probably can't start sending the file until it has successfully transcoded the file. Thus, you would have to buffer the file.

The two ways to buffer a file that I can think of are: memory, disk

Memory version, concat-stream or 'simple-concat'

var concat = require('simple-concat')

// ... stream.pipe(convert).pipe(concat(function (err, buf) { if (err) { res.writeHead(500) res.end() } else { res.writeHead(200) res.end(buf) } })) Depending on your architecture, you might want to cache to disk anyway:

stream.pipe(fs.createWriteStream('herp_derp')) .on('end', function () { // Or is it 'close'? res.writeHead(200) fs.createReadStream('herp_derp').pipe(res) }).on('error', function (err) { res.writeHead(500) res.end() }) If you use disk caching, maybe sox.js https://github.com/ArtskydJ/sox.js will work for you?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/ArtskydJ/sox-stream/issues/4#issuecomment-217227901