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

fix: don't swallow rejections in poll_value() #137

Merged

Conversation

nayeemrmn
Copy link
Collaborator

@nayeemrmn nayeemrmn commented Aug 8, 2023

Unblocks denoland/deno#20082.

Fixes three tangled issues:

  • JsRuntime::poll_value(promise) swallows non-awaited rejections returned from poll_event_loop() when promise is already settled.
  • JsRuntime::poll_value(promise) runs the event loop once even if promise is already settled.
  • JsRuntime::call_and_await() needs to explicitly call check_promise_rejections() at the end.

Some test expectations might need to add (in promise) to error strings. These are correct. There's at least one in CLI repo:

diff --git a/cli/tests/testdata/node/test.out b/cli/tests/testdata/node/test.out
index 8b7f0780f..3c54a15e8 100644
--- a/cli/tests/testdata/node/test.out
+++ b/cli/tests/testdata/node/test.out
@@ -147,7 +147,7 @@ error: Error: rejected from reject fail
     at [WILDCARD]
 
 ./node/test.js (uncaught error)
-error: Error: rejected from unhandled rejection fail
+error: (in promise) Error: rejected from unhandled rejection fail
   Promise.reject(new Error("rejected from unhandled rejection fail"));
                  ^
     at [WILDCARD]

@bartlomieju
Copy link
Member

@nayeemrmn what do you mean by

After fixing these, JsRuntime::call_and_await() doesn't account for non-awaited rejections.

How that should be handled by the embedder?

@nayeemrmn
Copy link
Collaborator Author

@nayeemrmn what do you mean by

After fixing these, JsRuntime::call_and_await() doesn't account for non-awaited rejections.

How that should be handled by the embedder?

I wrote it confusingly. It was a third issue introduced by fixing the first two but it's also fixed.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@bartlomieju bartlomieju merged commit 67c8ed0 into denoland:main Aug 8, 2023
@nayeemrmn nayeemrmn deleted the resolve-value-swallow-rejection branch August 8, 2023 08:44
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

Successfully merging this pull request may close these issues.

2 participants