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

Make sure to propagate a promise job's snapshot when handling abrupt completions #41

Merged
merged 4 commits into from
Jan 23, 2024

Conversation

andreubotella
Copy link
Member

Closes #35.

@andreubotella andreubotella changed the title Make sure to propage a promise job's snapshot when handling abrupt completions Make sure to propagate a promise job's snapshot when handling abrupt completions Mar 17, 2023
@andreubotella andreubotella force-pushed the fix-unhandled-rejection-context branch from 6ae967d to ac0cda4 Compare March 17, 2023 18:54
spec.html Outdated Show resolved Hide resolved
Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

With this change, I don't think we need HostCallJobCallback at all. And if we land on something like #16, we'd be able to remove HostMakeJobCallback and instead store directly on the PromiseCapability.

@andreubotella
Copy link
Member Author

andreubotella commented Mar 22, 2023

With this change, I don't think we need HostCallJobCallback at all. And if we land on something like #16, we'd be able to remove HostMakeJobCallback and instead store directly on the PromiseCapability.

True, I hadn't realized these promise jobs were the only users of HostCallJobCallback (other than FinalizationRegistry callbacks, which I guess should also be considered re. which context to use). I've now changed the PR to swap the context before calling HostCallJobCallback, and to not change that host hook at all.

I don't think it's yet fully clear which option to go for in #16, so I think getting rid of HostMakeJobCallback should be left for a follow-up PR when that is more settled.

Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Need to update CleanupFinalizationRegistry, otherwise LGTM

spec.html Outdated Show resolved Hide resolved
@jridgewell jridgewell force-pushed the fix-unhandled-rejection-context branch from 2909076 to 68bab19 Compare January 20, 2024 00:04
Copy link
Member

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Rebased this and resolved merge conflicts.

@jridgewell jridgewell merged commit 7b3bfb4 into tc39:master Jan 23, 2024
4 checks passed
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.

Bug in unhandledrejection context of promise rejected in next tick
3 participants