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

Reduce generates 'unhandledRejection' events despite catch #1501

Closed
erikerikson opened this issue Feb 6, 2018 · 7 comments

Comments

@erikerikson
Copy link

commented Feb 6, 2018

  1. What version of bluebird is the issue happening on?
    3.5.1 & branch associated with #1489

  2. What platform and version? (For example Node.js 0.12 or Google Chrome 32)

$node --version
v6.11.0
  1. Did this issue happen with earlier version of bluebird?
    Does not reproduce in 3.5.0

Description of issue:

Reproduction here:
https://github.com/erikerikson/chai-bluebird/tree/bb-1489-use-test

Executing:

const BbPromise = require('bluebird');

process.on('unhandledRejection', err => {
  console.log('###########################################################################');
  console.log(`UNHANDLED! ${err}`);
  console.log('###########################################################################');
});

BbPromise.reduce(
  [
    BbPromise.resolve('foo'),
    BbPromise.reject(new Error('reason')),
    BbPromise.resolve('bar')
  ],
  () => {},
  {}
)
  .catch(() => console.log('caught'))
  .then(() => console.log('after'))

Produces:

###########################################################################
UNHANDLED! Error: reason
###########################################################################
caught
after

Neither of the following produce this:

BbPromise.reduce(
  [
    BbPromise.resolve('foo'),
    BbPromise.resolve('bar')
  ],
  () => BbPromise.reject(new Error('reason')),
  {}
)
  .catch(() => console.log('caught'))
  .then(() => console.log('after'))
BbPromise.reduce(
  BbPromise.reject(new Error('reason')),
  () => {},
  {}
)
  .catch(() => console.log('caught'))
  .then(() => console.log('after'))

Perhaps related to #1468 #1487

@overlookmotel

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

This isn't entirely new to Bluebird v3.5.1.

With Bluebird v3.5.0, the following similar example creates an unhandled rejection:

BbPromise.reduce(
  [
    new BbPromise(resolve => setTimeout(resolve, 100)),
    BbPromise.reject(new Error('reason'))
  ],
  () => {},
  {}
)
  .catch(() => console.log('caught'))
  .then(() => console.log('after'))

I believe that the cause (in both v3.5.0 and v3.5.1) is that each input promise is dealt with in series. So the 2nd promise only has .then() called on it after the 1st promise resolves.

So a tick passes and the rejected promise has not been handled. An unhandled rejection!

A workaround would be to run the input array through Promise.all() before reducing. Since you only get a meaningful result out of Promise.reduce() if none of the Promises reject, this should give the same result, as long as the reduce handler doesn't have side effects.

@overlookmotel

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

This also creates an unhandled rejection for the same reasons (in both v3.5.0 and v3.5.1):

BbPromise.reduce(
    [ BbPromise.reject( new Error('reason') ) ] ,
    () => {},
    BbPromise.reject( new Error('reason2') )
).catch( () => {} );
@overlookmotel

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2018

Have submitted PR #1502 which fixes this. But I doubt will be merged as it's a bit of a hacky solution. @erikerikson if you're interested in getting this solved properly, I'd suggest building on my PR to make a cleaner solution.

@erikerikson

This comment has been minimized.

Copy link
Author

commented Feb 12, 2018

Thanks @overlookmotel . Using BbPromise.all should work perfectly.

The embarrassing truth about this issue is that it was the result of my attempting to find a boiled down reproduction of the error caused by an error I wrote into a pending promise "tracker". I have fixed that but discovered this behavior in the meantime.

I could see an argument for this not being a bug as easily as for it being one. Anyway, my hope was that at least a bit of value could result. It's seems a unlikely use case to be honest.

@aviranco

This comment has been minimized.

Copy link

commented Jun 17, 2018

I've experienced this issue as well. What is the status of this issue? the fix is planned to be merged?

@erikerikson Why this scenario that eventhough a promise is being handled it throws an unhandledRejection is not a bug?

@erikerikson

This comment has been minimized.

Copy link
Author

commented Jun 17, 2018

If you consider:
#1501 (comment)

The rejection occurs immediately but the inspection of the rejected promise by reduce isn't really scheduled until t+100ms.

In short the array and all of its promises, have been created and are the responsibility of code that is not reduce and it is asserting that reduce should pre-interact with all of them and then interact with them again in the expected process of executing the reduction function against each of them in serial.

The usual expectation with reduce is that a single iteration over the given iterable will occur. To catch rejections that occur prior to the reduction function being executed (and are thus placed on the event queue), a pre-iteration would be required. At least in theory, some iterations are stateful (consider a seek on a file moving a physical read head) and resetting is not a usual part of the contract. This ignores the performance impact and expectation mismatch.

So, I could see an argument that pre-inspection is appropriate to wipe up after users that aren't thinking through their expectations like I hadn't. On the other hand that would come at the cost of unnecessary instruction execution and potential side effects for users that had thought things through and appreciate the many interactions in these scenarios.

It isn't that it couldn't be considered a bug, it's simply that fixing it could be considered introducing a bug.

@petkaantonov

This comment has been minimized.

Copy link
Owner

commented May 25, 2019

Pre iteration is fine if it only checks for bluebird promise and enables the unhandled rejection suppression flag on them. That has no side effects.

This was referenced Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.