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

exception in mapSync causes fatal error #94

Closed
nyurik opened this Issue Jun 17, 2016 · 4 comments

Comments

Projects
None yet
2 participants
@nyurik
Contributor

nyurik commented Jun 17, 2016

The error thrown in the middle of the mapSync() cases application exception. Shouldn't mapSync handle that? Also, do I need to do on('error') after each stream transformation?

return new Promise(function (resolve, reject) {
    var s = fs.createReadStream(srcFile)
        .on('error', reject)
        .pipe(es.split())
        .on('error', reject)
        .pipe(es.mapSync(function (line) {
            ...
            throw new Error();
            ...
        }).on('error', reject)
        .pipe(fs.createWriteStream(outputFile))
        .on('error', reject)
        .on('finish', function () {
            resolve(outputFile);
        });
});
@dominictarr

This comment has been minimized.

Owner

dominictarr commented Jun 18, 2016

Unfortunately, there is no way other than catching on('error') for node streams to handle that.
(the appropiate way to handle that would depend entirely on what your map function actually does)
maybe put a try catch inside your map function if it's okay for the stream to continue.

@nyurik

This comment has been minimized.

Contributor

nyurik commented Jun 18, 2016

@dominictarr I think that since mapSync() expects a synchronous function, it should do the proper error handling - and wrap the call to the user func in a try{ call() } catch{emit('error')}. The user function should not handle its own errors by calling the stream's emit('error'), simply because otherwise you expect users to deal with errors in two ways - both sync and async (e.g. the stream breaks or the sync function fails)

@nyurik

This comment has been minimized.

Contributor

nyurik commented Jun 18, 2016

Fixed in v3.3.3

@nyurik nyurik closed this Jun 18, 2016

@nyurik

This comment has been minimized.

Contributor

nyurik commented Jun 18, 2016

Meh, probably should have added a unit test for this. @dominictarr if you have any spare cycles, i think it would be good to have a crashing unit test or two :)

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