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

What should the behavior of an exception inside a promise be? #5787

Open
WieserSoftware opened this issue Nov 13, 2018 · 3 comments
Open

What should the behavior of an exception inside a promise be? #5787

WieserSoftware opened this issue Nov 13, 2018 · 3 comments

Comments

@WieserSoftware
Copy link

Is this a bug report?

No/Maybe

I have a section of code (in typescript using "react-scripts": "2.1.1") that generates a promise that is used later. Inside that promise, I occasionally will get a bad object, and expect the promise to be rejected as a result of an object being undefined. Here's a pathological example, where I force the variable a to be undefined to illustrate the problem, which gives
Unhandled Rejection (TypeError): Cannot read property 'toLowerCase' of undefined

    const a: any = undefined;
    cached = new Promise(async (resolve, reject) => {
         const ret = a.toLowerCase();
         resolve(ret);
   });

On chrome this generates an exception at runtime when running the debug build, which prevents any further use of the app, but does not on edge.

If instead, I rewrite it like this, it also fails on chrome.

    const a: any = undefined;
    cached = new Promise(async (resolve, reject) => {
       try {
         const ret = a.toLowerCase();
         resolve(ret);
       } catch (e) {
            throw(e);
       }
   });

This too fails, making it look like anything thrown that derives from error causes the message:

    const a: any = undefined;
    cached = new Promise(async (resolve, reject) => {
       try {
         const ret = a.toLowerCase();
         resolve(ret);
       } catch (e) {
            throw new Error("fix chrome");
       }
   });

however if I rewrite it like this, it does not fail on chrome.

    const a: any = undefined;
    cached = new Promise(async (resolve, reject) => {
       try {
         const ret = a.toLowerCase();
         resolve(ret);
       } catch (e) {
            throw "fix chrome";
       }
   });
@Timer Timer added this to the 3.0 milestone Nov 13, 2018
@heyimalex
Copy link
Contributor

heyimalex commented Nov 13, 2018

They all fail on chrome and firefox for me with an uncaught exception. The issue is in using async functions for the promise constructor.

It's true that an async function will catch exceptions and return a rejected promise. It's also true that a promise constructor will catch exceptions and return a rejected promise. However, new Promise doesn't expect the constructor to return a promise, it expects it to either call resolve, call reject, or throw. The async function successfully turns the exception into a rejected promise and returns it, but the external promise remains unresolved; neither resolve nor reject has been called, and the constructor body didn't throw, it just returned a promise. Finally, the promise returned from the async function was rejected but never caught, so it gets elevated to the global uncaught promise handler, which is the exception you're actually seeing.

You can either remove the async, or remove the new Promise constructor and just use an async function directly. Mixing them like this isn't really idiomatic imo. Pretty confusing stuff!

@WieserSoftware
Copy link
Author

I'm not sure I follow what you mean by
new Promise doesn't expect the constructor to return a promise, it expects it to either call resolve, call reject, or throw.

Many of the examples don't resolve immediately (as that's the whole point). Imagine my toLowerCase were actually toLowerCaseAsync() above and required an await.

This example for instance
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function
or this from elsewhere in my code

   private async readAsDataUrl(response: Response): Promise<string> {
        const t = new Promise<string>(async (resolve, reject) => {
            if (response.ok) {

                const data = await response.blob();
                const fr = new FileReader();
                fr.onload = () => {
                    resolve(fr.result as string);
                };

                fr.readAsDataURL(data);
            }
        });

        return t;
    }

I can't really see how to do this without creating a promise with an async executor.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise#Syntax

@heyimalex
Copy link
Contributor

heyimalex commented Nov 14, 2018

It doesn’t have to call resolve or reject now, it just has to call one of them eventually. The executor doesn’t expect a promise to be returned; the type is (resolve, reject) => void, not (resolve, reject) => Promise. Whatever is returned is just ignored.

You generally only use new Promise to wrap non-promise interfaces, and then use that in an async function for better control flow. In that code snippet, you’d promisify the FileReader.onload in a new function, and then await that. I didn't delve too deep into the correctness of this code, but roughly like this.

async function readAsDataUrl(response: Response): Promise<string> {
  const blob = await response.blob();
  const result = await readDataUrlFromBlob(blob);
  return result;
}

function readDataUrlFromBlob(blob: Blob): Promise<string> {
  return new Promise<string>((resolve, reject) => {
    const fr = new FileReader();
    fr.onload = () => {
      resolve(fr.result as string);
    };
    fr.onerror = () => {
      reject(fr.error);
    }
    fr.readAsDataURL(blob);
  });
}

@iansu iansu modified the milestones: 3.0, 3.x Mar 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants