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

Gotcha: child_process.exec() error case doesn't give you access to stdout and stderr #197

Open
aseemk opened this issue Jan 15, 2014 · 3 comments
Labels

Comments

@aseemk
Copy link
Contributor

aseemk commented Jan 15, 2014

This isn't a Streamline bug per se — in fact, I think Streamline is doing the right thing — but it's definitely a gotcha to Streamline users. My peer @gasi ran into this.

Node's child_process.exec() takes a callback of the form (error, stdout, stderr). If there's an error, stdout and stderr are still passed, so you can e.g. log them.

Unfortunately, there's no way to access those things from Streamline. You can generalize the issue to:

try {
  var results = someFunc(_);
} catch (err) {
  // no access to results
}

This of course makes sense: any synchronous language wouldn't set results if the someFunc() call threw an error. The correct thing would be if the err object had the results as a property.

That's in fact how we asked Mikeal to solve it for node-request: request/request#28

We can make a similar request to Node core, but I'd doubt they'd make this kind of change, since it's unnecessary from regular JS.

For now, we'll solve it on our end by using a wrapper function.

function wrapper(cb) {
    someFunc(function (err, results) {
        if (err) err.results = results;
        cb(err, results);
    });
}

(And we'll need another wrapper that's a regular Streamline function if we want futures, until #181 is implemented.)

But perhaps this kind of problem can be built solved natively by Streamline somehow? Ideas:

  1. At the very least, expose a streamline/child_process wrapper like streamline/fs?

  2. In the internal callback handler, in the error case, if there are additional arguments that are passed, tack them onto the error object? E.g. err._result (or err._results if [_] was specified).

    This is definitely a start, but still not great, as the developer has to know the name _results rather than whatever variable name they might be using in the non-error case.

  3. Some other way??

Thanks as always Bruno!

@bjouhier
Copy link
Member

I globally agree with your analysis but I would not introduce special support for this type of API directly in the streamline runtime. It seems to be a rather singular case and there is an easy workaround.

On the other hand, a streamline-child-process helper package would help.

@aseemk
Copy link
Contributor Author

aseemk commented Feb 9, 2014

I think that's very reasonable. =)

@aseemk
Copy link
Contributor Author

aseemk commented Sep 13, 2014

For anybody else that comes across this, here's the (CoffeeScript) wrapper we wrote for ourselves:

#
# Node's native child_process callbacks pass `stdout` and `stderr` even in
# case of errors, which we can't access via Streamline "sync" error handling.
# (Just like a synchronous try-catch in any language.)
#
# So we wrap child_process callback-based methods here to attach those
# `stdout` and `stderr` results onto the error object in case of errors.
#

child_process = require 'child_process'

# Helper to wrap the given child_process method. Also exports it.
wrap = (method) =>
    exports[method] = (args...) ->
        # wrap the callback, which should be the last arg:
        [..., callback] = args
        args[args.length - 1] = (err, stdout, stderr) ->
            err?.stdout = stdout
            err?.stderr = stderr
            callback.apply this, arguments

        # then relay the call:
        child_process[method].apply child_process, args

wrap 'exec'
wrap 'execFile'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

2 participants