Throw an error when we receive data from stderr

[gillesdemey]

Fixes #19

This might be a breaking change for some folks who were not expecting data from stderr to emit an error.

I've noticed that sox writes warnings to stderr as well, so that might make it slightly tricky for users to figure out what to ignore.

Some examples from what sox might output as a warning:

rec WARN formats: can't set 1 channels; using 2
rec WARN wav: Length in output .wav header will be wrong since can't seek to fix it

We could reject output that contains WARN and not propagate those? I'm not sure what the behaviour is of rec and arec.

[mreinstein]

lol, I guess we were working on this at the exact same moment. :)

[mreinstein]

I've noticed that sox writes warnings to stderr as well, so that might make it slightly tricky for users to figure out what to ignore.

2 options I can think of:

• We could buffer the data from stderr, detect when stderr closes, and forward that buffered data to stdout as an emitted error when the stream actually closes.
• change the public interface to allow direct access to the stderr stream, and bump the version number to 1.0.0

[mreinstein]

I wonder if spawn reliably returns a non-zero exit code when sox/arecord/etc close due to failure?

[gillesdemey]

lol, I guess we were working on this at the exact same moment. :)

Heh, sorry had this sitting on a separate branch to give this a try and decided to create a PR where we could discuss this 🙇

Option 1 sounds interesting, I'll give that a try and see how that goes Option 2 doesn't address the false positives, unfortunately

I wonder if spawn reliably returns a non-zero exit code when sox/arecord/etc close due to failure?

Spawn returns the exit code from the process, so I think the real question is wether or not rec and its ilk return proper exit codes

[mreinstein]

the real question is wether or not rec and its ilk return proper exit codes

I've attached a listener in my local copy:

cp.on('close', function(code) {
  console.log('exit code:', code)
})

If I start my program with the microphone unplugged, I get an exit code of 2. If I unplug the microphone mid-recording, I get an exit code of 0. So it seems this is not a reliable way to detect errors in sox. On top of that, apparently different shells can result in different exit codes so it's not portable across posix environments anyway. :-/

[mreinstein]

I don't think there's a non-brittle way to know that something emitted on stderr is an error that will kill the stream vs simply a warning.

If that brittleness of checking for WARN must exist, I'd rather have it in my userland code than bake it into node-record-lpcm16.

I'm fine with simply exposing this error event as-is and letting the caller decide what to do.

[gillesdemey]

I agree that it makes most sense to let the user deal with anything that comes from stderr; in which case I think this PR is good to go.

I also think this warrants a major version upgrade since some folks might be getting errors that are actually warnings.

[mreinstein]

in which case I think this PR is good to go.

I agree, PR LGTM 👍 !

[mreinstein]

I also think this warrants a major version upgrade

Actually I don't think your PR breaks the API in any way. It's a new addition (emitting the error event.) Unless I'm missing something.

[gillesdemey]

If you are using it in combination with something like pumpify it will close the all streams when an error occurs in any of them.

[mreinstein]

ah you're right. I guess because stderr errors weren't being forwarded anywhere before they would not affect a set of piped streams.