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

Promise.prototype.finally is not aligned with the most current spec draft #161

Open
MapTo0 opened this issue Mar 11, 2020 · 1 comment
Open

Comments

@MapTo0
Copy link

MapTo0 commented Mar 11, 2020

According to draft 4 of the Promise.prototype.finally spec , the callback parameter might not be callable.

Spec: https://tc39.es/proposal-promise-finally/
Similar issues: stefanpenner/es6-promise#336

@Jackman3005
Copy link

Jackman3005 commented Jul 31, 2023

OMG. We have hundreds of Sentry issues that I've been debugging for weeks and weeks on and off, made so much worse by the really poor unhandled promise rejection stack I was getting in our react-native environment made this immensely difficult to track down. Turns out I was trusting the types too much and thought it was safe to do

myPromiseFn().finally()

Because the types say that the argument to finally is optional. I was doing this throughout my code because I was otherwise getting ESLint warnings saying I needed to await the promise that was returned. Doing this was my way of communicating that I know what I'm doing and do not intend to await this promise (in a useEffect() for example).

tl;dr

This library requires a function to be provided to finally and if not provided it throws with:

TypeError: undefined is not a function

Types Expectation

The types I find when clicking through to the definition of finally() are as follows:

interface Promise<T> {
    finally(onfinally?: (() => void) | undefined | null): Promise<T>
}

These types are provided directly from Typescript.

SEO for others in pain

Warning message I was receiving:

Possible Unhandled Promise Rejection (id: 0): TypeError: undefined is not a function

Sentry polyfills promise to make it nearly identical to the normal react-native promise, but also adds in sentry tracking of unhandled promise exceptions. This was the specific usage of this library I found.

Trying to find the original stack trace of unhandled promise rejections in this particular scenario was gruelingly difficult as the stack never included relevant code, just library code like the following:

(When Sentry was normally polyfilling)

TypeError: undefined is not a function
    at anonymous (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:125276:31)
    at tryCallOne (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:125101:16)
    at anonymous (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:125182:27)
    at apply (native)
    at anonymous (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26192:26)
    at _callTimer (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26111:17)
    at _callReactNativeMicrotasksPass (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26141:17)
    at callReactNativeMicrotasks (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26304:44)
    at __callReactNativeMicrotasks (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19761:46)
    at anonymous (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19573:45)
    at __guard (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19745:15)
    at flushedQueue (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19572:21)
    at callFunctionReturnFlushedQueue (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19557:33)

(When Sentry was set to enabled: false)

WARN  Possible Unhandled Promise Rejection (id: 5):
TypeError: undefined is not a function
TypeError: undefined is not a function
    at anonymous (/Users/distiller/react-native/sdks/hermes/build_iphonesimulator/lib/InternalBytecode/InternalBytecode.js:339:28)
    at tryCallOne (/Users/distiller/react-native/sdks/hermes/build_iphonesimulator/lib/InternalBytecode/InternalBytecode.js:53:16)
    at anonymous (/Users/distiller/react-native/sdks/hermes/build_iphonesimulator/lib/InternalBytecode/InternalBytecode.js:139:27)
    at apply (native)
    at anonymous (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26192:26)
    at _callTimer (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26111:17)
    at _callReactNativeMicrotasksPass (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26141:17)
    at callReactNativeMicrotasks (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:26304:44)
    at __callReactNativeMicrotasks (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19761:46)
    at anonymous (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19573:45)
    at __guard (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19745:15)
    at flushedQueue (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19572:21)
    at callFunctionReturnFlushedQueue (http://127.0.0.1:19000/index.bundle?platform=ios&dev=true&hot=false:19557:33)

Libs we are using:

    "expo": "~47.0.14",
    "sentry-expo": "~5.0.0",
    "@sentry/react-native": "4.2.2",
    "react-native": "0.70.8",

Identifying the offending line of code

I tried a few different StackOverflow answers for trying to find the real stack trace of the promise that was causing the issue I was seeing. As noted the stack trace that is printed does not end up leading to any code I have written, which made it questionable whether it was my fault or potentially some other library causing the issue.

After trying to use bluebird as suggested here I curiously found that the warning went away. Now I know it was because Bluebird promise implementation does not require a finally parameter!

In the end, I did the following to identify the offending line of code (sorry this isn't a patch-package patch as we do not use that lib See patch package below)

Manually add three lines to these two files

File 1: node_modules/promise/setimmediate/core.js
Add the following line of code in the function Promise(fn) {... function (line 61 for me).

  this._stack = (new Error()).stack;

File 2: node_modules/promise/setimmediate/rejection-tracking.js
Inside the enable function where a setTimeout is called (line 47 for me) with:

onUnhandled.bind(null, promise._51)

Replace it with:

onUnhandled.bind(null, promise._51, promise)

Then in the onUnhandled function just add a 2nd parameter "promise" and console log there and you'll see the _stack field printed out with the original location where the Promise was created.

  function onUnhandled(id, promise) { // line 60 for me
    console.log("rejection-tracking:61 - onUnhandled", id, promise)
Or use this Patch with `patch-package`
diff --git a/node_modules/promise/setimmediate/core.js b/node_modules/promise/setimmediate/core.js
index a84fb3d..1389b85 100644
--- a/node_modules/promise/setimmediate/core.js
+++ b/node_modules/promise/setimmediate/core.js
@@ -58,6 +58,14 @@ function Promise(fn) {
   if (typeof fn !== 'function') {
     throw new TypeError('Promise constructor\'s argument is not a function');
   }
+  if(!__DEV__) {
+    console.warn('WARNING - Promise Stack Trace Debugging is NOT recommended for production code.');
+  }
+  const stackTraceLimit = Error.stackTraceLimit
+  Error.stackTraceLimit = Infinity
+  this._stack = (new Error()).stack;
+  Error.stackTraceLimit = stackTraceLimit
+
   this._40 = 0;
   this._65 = 0;
   this._55 = null;
diff --git a/node_modules/promise/setimmediate/rejection-tracking.js b/node_modules/promise/setimmediate/rejection-tracking.js
index 10ccce3..75f2fd1 100644
--- a/node_modules/promise/setimmediate/rejection-tracking.js
+++ b/node_modules/promise/setimmediate/rejection-tracking.js
@@ -44,7 +44,7 @@ function enable(options) {
         displayId: null,
         error: err,
         timeout: setTimeout(
-          onUnhandled.bind(null, promise._51),
+          onUnhandled.bind(null, promise._51, promise),
           // For reference errors and type errors, this almost always
           // means the programmer made a mistake, so log them after just
           // 100ms
@@ -57,7 +57,8 @@ function enable(options) {
       };
     }
   };
-  function onUnhandled(id) {
+  function onUnhandled(id, promise) {
+    console.warn('Unhandled Promise Rejection', id, promise)
     if (
       options.allRejections ||
       matchWhitelist(

Although I got a bundled stack trace I was still able to track it down when the code was not minified.

NOTE: This is NOT performant and SHOULD NOT be included in any production builds as calling (new Error()).stack on every promise constructor will slow things down a lot. For debugging this is not an issue though.

I'd love for someone to tell me how I could have done this an alternative way as I feel it is ridiculous I had to go through these steps.

Edit: I decided to create a patch-package patch to make it easier to do this again in the future:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants