Fix unhandled promise rejection in onceWithCleanup #2833
Conversation
lib/promise_utils.js
Outdated
| @@ -67,7 +67,7 @@ function onceWithCleanup (emitter, event, { timeout = 0, checkCondition = undefi | |||
| }) | |||
| } | |||
|
|
|||
| task.promise.finally(() => emitter.removeListener(event, onEvent)) | |||
| task.promise.finally(() => emitter.removeListener(event, onEvent)).catch((err) => {}) | |||
There was a problem hiding this comment.
This does not seem right. You're silently letting this fail.
There was a problem hiding this comment.
The promise returned by finally is a copy off the promise the task itself will return. This is why there is a unhandled promise rejection in the first place. No one is handling the promise returned by finally because it is also returned by the promise in task.
There was a problem hiding this comment.
Altho this does not work with the linter. Maybe promise.catch(() => {}).finally(() => ...) would be better?
There was a problem hiding this comment.
You’re right, I missed that. It’s a chained promise, so it should be catching error removing the event (if I’m not mistaken)
|
@extremeheat Changed it |
|
Why did you change it ? I said you were right. |
|
I changed it because the old version caused a lint error. This version works. |
From MDN:
So finally will throw an unhandled promise rejection if the task promise rejects. Causing a un-catchable promise rejection on the onceWithCleanup function