Catch errors thrown by onInclude

[LinusU]

When using the .then($1, $2) function, any errors thrown from $1 will not be delegated to $2. Changing to .then($1).catch($2) makes sure that even errors from $1 gets delegated to $2.

Found by this proposed standard rule: standard/eslint-config-standard#129

[coveralls]

Coverage remained the same at 86.228% when pulling 6f2ac5f on LinusU:patch-1 into 402c1d0 on jprichardson:master.

[LinusU]

Seems like one of the builds hanged, would you mind restarting it? ☺️

[RyanZim]

Build retriggered.

The reason this is built this way is that we're calling cb inside .then. cb is a user-provided callback function in some cases. If the user has a bug in their code, causing an error to be thrown during the execution of their callback, we need to handle it correctly. With your proposed implementation, when cb throws, the error will be caught by the .catch block, which will call cb again.

The native fs module handles errors in user-provided callbacks by exiting the process there and then. When unhandledRejections begin killing the Node process, our behavior will be on par with Node.js core fs.

However, your PR did bring to my attention that our implementation is slightly incorrect, since cb isn't the only call in the .then block; so there's a possibility of an internal fs-extra error causing an unhandledRejection. The function should read:

function handleFilter (onInclude, destStat, src, dest, opts, cb) {
   Promise.resolve(opts.filter(src, dest))
   .then(include => {
     if (include) {
       if (destStat) return onInclude(destStat, src, dest, opts, cb)
       return onInclude(src, dest, opts, cb)
     }
   })
   .then(() => cb(), error => cb(error))
}

[LinusU]

Hmm, I think the code that you proposed might call the callback cb twice? Once as soon as the call to onInclude returns, and then later when onInclude is done with its async work.

There is another problem as well, if cb throws, that error will reject the resulting promise, instead of emitting an uncaughtException event on the process, and then terminating the process.

Unfortunately, it's quite hard to mix promises and callbacks, especially when trying to call a callback from a function that uses promises.

My personal recommendation, and I would be happy to help with this if you want to, is to just use Promises internally, and then convert to callbacks only at the outmost API-layer. This "callbackification" can then be handled in a proper way easily, using something like this: https://nodejs.org/api/util.html#util_util_callbackify_original

[LinusU]

Didn't find any proper callbackification package for Node 6.x, so I ported the built-in one from Node 10 to also work on Node.js 6: https://github.com/LinusU/util-callbackify

If we use that one it will handle all the rejections properly, including when errors are thrown from the callback.

[RyanZim]

Hmm, I think the code that you proposed might call the callback cb twice? Once as soon as the call to onInclude returns, and then later when onInclude is done with its async work.

You're right, I wasn't looking closely at the code here.

There is another problem as well, if cb throws, that error will reject the resulting promise, instead of emitting an uncaughtException event on the process, and then terminating the process.

It will be an unhandledRejection, which will terminate the process in future versions. It's as close as we can get IMO.

My personal recommendation, and I would be happy to help with this if you want to, is to just use Promises internally, and then convert to callbacks only at the outmost API-layer.

This might be a good approach. For converting back to callbacks, we'd use https://github.com/RyanZim/universalify since all fs-extra supports both promises and callbacks, and it's already a dependency. @manidlou what are your thoughts on migrating copy to promises internally?

Regardless what we decide, I don't think this is a giant rush since we've never seen a problem as a result of this in the real world.

[manidlou]

@RyanZim I'm fine with migrating callbacks to promises internally. I'd just say if we do it for copy we should do it for the entire codebase to keep it consistent and If we decide to do this, we can do it incrementally.

[RyanZim]

Given that we've never seen any problems here in the real world, I'm gonna close this out.