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

Failure causes infinite loop #4

Closed
MGlauser opened this issue May 5, 2016 · 4 comments
Closed

Failure causes infinite loop #4

MGlauser opened this issue May 5, 2016 · 4 comments
Labels

Comments

@MGlauser
Copy link

MGlauser commented May 5, 2016

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
Copy link
Owner

ArtskydJ commented May 5, 2016

Whoops, good catch!

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

@ArtskydJ ArtskydJ added the bug label May 5, 2016
@MGlauser
Copy link
Author

MGlauser commented May 5, 2016

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 #4 (comment)

@ArtskydJ
Copy link
Owner

ArtskydJ commented May 5, 2016

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
Copy link
Author

MGlauser commented May 5, 2016

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 #4 (comment)

ArtskydJ added a commit that referenced this issue May 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants