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

unhandledrejection is not working for anonymous async function promises. Some code works in nodejs #16928

Closed
Lesiuk opened this issue Dec 4, 2022 · 17 comments · Fixed by #19235
Assignees
Labels
needs investigation requires further investigation before determining if it is an issue or not

Comments

@Lesiuk
Copy link

Lesiuk commented Dec 4, 2022

Deno (1.18.3)

import {delay} from "std/async/delay.ts";

globalThis.addEventListener("unhandledrejection", (error) => {
    console.error('unhandledRejection', error);
    e.preventDefault();
});

(async () => {
    throw 'failed';
})();


await delay(5_000);
console.log('success');

Node (19.1.0)

process.on('unhandledRejection', (error) => {
    console.error('unhandledRejection', error);
});

(async () => {
    throw 'failed';
})();

await new Promise((resolve) => setTimeout(resolve, 5_000));
console.log('success');

Result:
error: Uncaught "failed"

Expected:
success

@Lesiuk Lesiuk changed the title unhandledrejection is not working for anonymous function promises but works in nodejs. unhandledrejection is not working for anonymous async function promises, but works in nodejs. Dec 4, 2022
@Lesiuk Lesiuk changed the title unhandledrejection is not working for anonymous async function promises, but works in nodejs. unhandledrejection is not working for anonymous async function promises. Some code works in nodejs Dec 4, 2022
@bartlomieju
Copy link
Member

This works perfectly fine for me on Deno 1.28.3:

import {delay} from "https://deno.land/[email protected]/async/delay.ts";

globalThis.addEventListener("unhandledrejection", (e) => {
    console.error('unhandledRejection', e);
    e.preventDefault();
});

(async () => {
    throw 'failed';
})();


await delay(5_000);
console.log('success');

Handling of unhandledrejection was added in Deno v1.24, so earlier versions are not able to handle it properly.

@Lesiuk
Copy link
Author

Lesiuk commented Dec 4, 2022

@bartlomieju 1.18.3 was a typo, I mean 1.28.3

I checked it and you are right. It works when running from empty directory. But it does not with my deno.lock file.

I created a repository which reproduces it.

[email protected]:Lesiuk/deno-unhandledrejection-reproduction.git

image

@Lesiuk
Copy link
Author

Lesiuk commented Dec 4, 2022

@bartlomieju
image

@Lesiuk
Copy link
Author

Lesiuk commented Dec 4, 2022

Even if I refresh the lock file, it still doesn't work.

@bartlomieju
Copy link
Member

Interesting, I can indeed reproduce it with a lock file.

@bartlomieju bartlomieju reopened this Dec 4, 2022
@bartlomieju bartlomieju added the needs investigation requires further investigation before determining if it is an issue or not label Dec 4, 2022
@bartlomieju bartlomieju self-assigned this Dec 4, 2022
@bartlomieju
Copy link
Member

After some debugging and discussion with @dsherret I pinpointed that this is caused by npm: imports, which suggests that it's related to Node compatibility layer. I will debug it further, as it's possibly related to #16628

@bartlomieju
Copy link
Member

Digged a bit more and this is indeed coming from the Node compatibility layer, see:

  globalThis.addEventListener("unhandledrejection", (event) => {
      if (process.listenerCount("unhandledRejection") === 0) {
        // The Node.js default behavior is to raise an uncaught exception if
        // an unhandled rejection occurs and there are no unhandledRejection
        // listeners.
        if (process.listenerCount("uncaughtException") === 0) {
          throw event.reason;
        }

        event.preventDefault();
        uncaughtExceptionHandler(event.reason, "unhandledRejection");
        return;
      }

      event.preventDefault();
      process.emit("unhandledRejection", event.reason, event.promise);
    });

We're hitting the:

        if (process.listenerCount("uncaughtException") === 0) {
          throw event.reason;
        }

branch which causes the exception to be raised. I'm not sure what would be the best course of action here, @dsherret @cjihrig thoughts?

@bartlomieju
Copy link
Member

Related #16943

@Lesiuk
Copy link
Author

Lesiuk commented Dec 15, 2022

@bartlomieju Do you have any idea how we could fix this?

@bartlomieju
Copy link
Member

bartlomieju commented Dec 15, 2022

We've discussed this and still debating how to do it without having to introduce a special API for Node compat layer, however we don't have any good ideas, so we might have to add a special API. This is still on my radar, but I had to switch to work on the release. I'll try to get it fixed before end of the year.

@kt3k
Copy link
Member

kt3k commented Dec 18, 2022

How about adding a special API to get listener count of EventTarget?

const listenerCount = Symbol("listenerCount"); // only available to Deno internals
class EventTarget {
  ...
  /** Gets the count of listeners */
  [listenerCount](type: string)]: number {...}
}

And use it like:

if (process.listenerCount("uncaughtException") === 0 && globalThis[listenerCount]("unhandledrejection") === 0) {
  throw event.reason;
}

@piscisaureus
Copy link
Member

piscisaureus commented Dec 18, 2022

Here's an idea (JK!):

import { delay } from "https://deno.land/[email protected]/async/delay.ts";

globalThis.addEventListener("unhandledrejection", (e) => {
  console.error("unhandledRejection");
  e.preventDefault();
});

window.addEventListener("error", (ev) => {
  console.error("snatched error:", ev.error);
  ev.preventDefault();
});

(async () => {
  throw "failed";
})();

await delay(2_000);
console.log("success");

output:

🦕 deno run -A unhandled.ts
snatched error: failed
success

@bartlomieju
Copy link
Member

@piscisaureus both these event listeners are already installed by std/node so that's not really a viable solution. @kt3k's solution should work, but I'm worried about edge cases (that I admin I haven't come up with) that might not be sufficient - ideally we want to do "Node-style" handling at the very end, to let all user code event listeners handle it first.

@kt3k
Copy link
Member

kt3k commented Dec 19, 2022

@bartlomieju

ideally we want to do "Node-style" handling at the very end, to let all user code event listeners handle it first.

That might also work. Do you suggest special event registration API like .addLastEventListener(type: string)? Or maybe define a special event which will be triggered after unhandledrejection, like afterunhandledrejection (accessible only from the internals)?

@bartlomieju
Copy link
Member

@bartlomieju

ideally we want to do "Node-style" handling at the very end, to let all user code event listeners handle it first.

That might also work. Do you suggest special event registration API like .addLastEventListener(type: string)? Or maybe define a special event which will be triggered after unhandledrejection, like afterunhandledrejection (accessible only from the internals)?

I was suggesting a private API like: Deno[Deno.internal].node.unhandledRejectionHandler

@bartlomieju
Copy link
Member

Note to self: error, beforeunload and unload events most likely suffer from the same problem.

@bartlomieju
Copy link
Member

This should be much more approachable now with #17724 landed

bartlomieju added a commit that referenced this issue May 24, 2023
…mports (#19235)

This commit fixes emitting "unhandledrejection" event when there are
"node:" or "npm:" imports. 

Before this commit the Node "unhandledRejection" event was emitted
using a regular listener for Web "unhandledrejection" event. This
listener was installed before any user listener had a chance to be 
installed which effectively prevent emitting "unhandledrejection" 
events to user code.

Closes #16928
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs investigation requires further investigation before determining if it is an issue or not
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants