Preserve rejection state when using a .then reject handler

[Jessidhia]

On any Promises/A+ implementation, throwing in the reject handler will propagate the rejection.

[medikoo]

@Kovensky I don't think we should do that.

Returned promise is not consumed anywhere, and if one relies on implementation that notifies about unhandled errors, then it'll have its console polutted with tons of "Unhandled error .." reports.

[Jessidhia]

@medikoo It seems I misunderstood the design of this function, so this PR was indeed incorrect (it seems to convert async functions into sync functions?), but I also think you are misunderstanding what finally does, especially as that is your preferred chaining method. What any of the chaining methods do is, they do not "attach a callback" to the Promise, but create a new Promise. That is, you are forking the Promise chain.

In the case of finally, it is a Promise that will run the callback and preserve rejection if the input is rejected. Both Bluebird's documentation and the stage-0 proposal document that behaviour. The finally polyfill is almost literally, @@species support aside, this.then(v => Promise.resolve(callback()).then(() => v), e => Promise.resolve(callback()).then(() => { throw e }).

Also, in your hasFinally branches, the onFailure is always called -- on both success and failure; and due to the previous paragraph's consequences, the rejection case will always trigger an unhandledPromiseRejection event on bluebird/v8, even if the caller has a try/catch or would have used .catch.

If the memoized function does return a Promise when called, but memoizee doesn't / can't modify the return value, then you can just use .then(onSuccess, onFailure), with no re-throwing. As long as the user has called .then or .finally (i.e. created their own fork), debugging tools will see their fork as missing the .catch.

I am not sure on the overall architecture of memoizee, but, as an example, if I were to "hand-memoize" a single-argument Promise-based function, while transparently keeping the rejection-removes-from-cache behaviour, this is how I would do it:

---

/*

Only need one storage -- why store resolved values, it can just return the same Promise that can be forked however many times the caller needs. Settled Promises just call any .then callback immediately.

*/
handMemoizedAsyncOp.cache = new Map() // or WeakMap, if arg is object
// any -> Promise<any>
function handMemoizedAsyncOp (arg) {
  const memoized = handMemoizedAsyncOp.cache.get(arg)
  if (memoized != null) {
    // it's a Promise that is either not settled yet, or is resolved
    return memoized
  }

  /*

The memoizer doesn't care what happens on success -- it will be correctly cached as a promise resolved by asyncOp in that case.

It only has to handle rejection, deleting itself from the cache; future callers will thus not see a cached promise and will make a new request instead.

Re-throw the rejection value to ensure that the rejection can be observed (and caught) by anyone that received a copy of this Promise.

Not rethrowing will not only prevent caller-attached .catch from running, it will run any caller-attached .then with an undefined argument!

Note that return Promise.reject(e) is, under Promises/A+, exactly the same thing as throw e as far as can be observed.

  */
  const aPromise =
    asyncOp(arg)
    .catch(e => { handMemoizedAsyncOp.cache.delete(arg); throw e })
  // store a Promise that knows to remove itself on rejection
  handMemoizedAsyncOp.cache.set(arg, aPromise)
  // return the cached Promise
  return aPromise
}

---

All this, of course, depends on a Promises/A+ compliant implementation. If it's not Promises/A+, I have no idea :D

[medikoo]

finally is preferred simply to not register error handler on a promise. We don't want to mute eventual promise rejection if by a chance it was not handled by promise consumer.

Also, in your hasFinally branches, the onFailure is always called -- on both success and failure

Yes, it's totally intended and does no harm, see this code comment: https://github.com/medikoo/memoizee/blob/master/ext/promise.js#L39

Now concerning issues related to specific implementations:

• V8, native promises do not implement finally, therefore this flow is used in V8 -> https://github.com/medikoo/memoizee/blob/master/ext/promise.js#L64-L68
  It will mute eventual rejection, but due to limited implementation we can't do anything about that.
• Bluebird: I'm not sure if I understand what you write properly, but do you mean that using finally triggers unhandledPromiseRejection event on bluebird (?) If it's the case, then I see it more as a Blubird bug, still feel free to elaborate on that.
  Maybe the issue is that done doesn't work cleanly in bluebird (?) (see this note: https://github.com/medikoo/memoizee#important-notice-on-internal-promises-handling) if that's the case, please use promise: 'then' mode.

As an additional note: It would be too invasive if memoizee will return different promise object than one returned by original function. I'm sure for many it would be totally unexpected. Therefore we cannot chain promise for our internal needs, re-throw eventual error, and return its result

[Jessidhia]

@medikoo v8 will probably have finally eventually as there is a Promise.prototype.finally proposal; here is its polyfill: https://github.com/ljharb/proposal-promise-finally/blob/master/polyfill.js

I didn't even know Bluebird had a .done since it's not documented as a Promise method. It's also explicitly deprecated (it behaves like jQuery 1's .done).

Thinking more about it, with the restriction that you can't modify the returned Promise, the branch that you linked is actually the only correct branch, regardless of .finally. The only case it will silence errors is if the caller doesn't do anything with the returned Promise. Remember, you're only silencing the error on your fork of the chain, not on the chain the caller received. Rejections only count as handled if all forks of a chain handle them.

Node REPL session with native Promise:

> process.on('unhandledRejection', function (reason) { console.log('unhandled rejection', reason) }), undefined
undefined
> const memoizee = require('memoizee')
undefined
> const test = memoizee(a => Promise.reject(a), { promise: true })
undefined
> test('unused promise, the .then counts as a rejection handler')
Promise {
  <rejected> 'unused promise, the .then counts as a rejection handler' }
> test('used promise, unhandled').then(() => {})
Promise { <pending> }
> unhandled rejection used promise, unhandled
> test('caught rejection').catch(() => {})
Promise { <pending> }
>

Node REPL session with Bluebird Promise:

> global.Promise = require('bluebird'); undefined
undefined
> process.on('unhandledRejection', function (reason) { console.log('unhandled rejection', reason) }); undefined
undefined
> const memoizee = require('memoizee')
undefined
> const test = memoizee(a => Promise.reject(a), { promise: true })
undefined
> test('unused promise')
Promise {
  _bitField: 16777216,
  _fulfillmentHandler0: 'unused promise',
  _rejectionHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined }
> Fatal unused promise

Crashes with a Fatal error (process.abort()) as you call .done on it by default. Don't do that.

Again using Bluebird, but with promise: 'then':

> global.Promise = require('bluebird'); undefined
undefined
> process.on('unhandledRejection', function (reason) { console.log('unhandled rejection', reason) }); undefined
undefined
> const memoizee = require('memoizee')
undefined
> const test = memoizee(a => Promise.reject(a), { promise: 'then' })
undefined
> test('unused promise')
Promise {
  _bitField: 16777216,
  _fulfillmentHandler0: 'unused promise',
  _rejectionHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined }
> unhandled rejection unused promise
unhandled rejection unused promise
> test('used promise').then(() => {})
Promise {
  _bitField: 0,
  _fulfillmentHandler0: undefined,
  _rejectionHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined }
> unhandled rejection used promise
unhandled rejection used promise
unhandled rejection used promise
> test('caught rejection').catch(() => {})
Promise {
  _bitField: 0,
  _fulfillmentHandler0: undefined,
  _rejectionHandler0: undefined,
  _promise0: undefined,
  _receiver0: undefined }
> unhandled rejection caught rejection
unhandled rejection caught rejection

When the promise is not used, there are two unhandled rejection errors: one on the Promise you create with promise.then(function (result) { nextTick(onSuccess.bind(this, result)); });, one on the Promise you create with promise.finally(function () { nextTick(onFailure); });. Only the ends of Promise chains cause unhandled rejection errors.

When the promise is used, I added another end to the chain that doesn't handle it, so now I get 3 rejection errors.

When the promise has a .catch attached, it will catch the rejection on my branch of the Promise, but the two Promises you are creating in your handler don't, so you still cause unhandled rejection errors!

Remember, you are never "attaching handlers" to a Promise, you are always constructing new Promises that merely depend on the previous Promise.

This incorrect behaviour is not unique when using Bluebird; it will also happen when using any other Promises/A+ implementation that has a .finally... like v8 Promises with (a simplified version of) the Promise.prototype.finally polyfill:

> process.on('unhandledRejection', function (reason) { console.log('unhandled rejection', reason) }); undefined
undefined
> Promise.prototype.finally = function (cb) { if (typeof cb !== 'function') { cb = function () {}; } return this.then(v => { Promise.resolve(cb()).then(() => v) }, e => { Promise.resolve(cb()).then(() => { throw e }) }) }
[Function]
> const memoizee = require('memoizee')
undefined
> const test = memoizee(a => Promise.reject(a), { promise: true })
undefined
> test('unused promise')
Promise { <rejected> 'unused promise' }
> unhandled rejection unused promise
unhandled rejection unused promise
> test('used promise').then(() => {})
Promise { <pending> }
> unhandled rejection used promise
unhandled rejection used promise
unhandled rejection used promise
> test('handled promise').catch(() => {})
Promise { <pending> }
> unhandled rejection handled promise
unhandled rejection handled promise

[medikoo]

@medikoo v8 will probably have finally

Yes, probably at some point, but that's not really relevant to this discussion

I didn't even know Bluebird had a .done since it's not documented as a Promise method. It's also explicitly deprecated (it behaves like jQuery 1's .done).

done is the only way to access resolved value without side effects (error swallowing, unnecessary chain extension), and all good implementation should have it. Still it's true there are very different point views on it in, and you'll find people that'll tell you the opposite.

Rejections only count as handled if all forks of a chain handle them.

That's indeed the thing I've overseen when trying then & finally pair.
The internally created promise (via then) will be rejected, and as no error handler to it is attached, it wil report Unhandled rejection (doesn't matter that on parent promise error could have been handled)
I will fix it so no finally method is used with then, it will make solution limited, but there's probably no clean workaround when no done is available.

[medikoo]

I've published update as v0.4.1, and I've also opened issue to introduce solution you suggested as one of the options -> #60

[Rush]

done is the only way to access resolved value without side effects

With bluebird one can use http://bluebirdjs.com/docs/api/reflect.html in order to find the resolved value without side effects.

[medikoo]

@Rush I would like not to dive into library specific API's, as there are tens of promise libraries out there. done is pretty standard and I think it's as far as I would want to go.