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

Vows do not report unhandled rejections #10576

Open
mhofman opened this issue Nov 26, 2024 · 0 comments · May be fixed by #10686
Open

Vows do not report unhandled rejections #10576

mhofman opened this issue Nov 26, 2024 · 0 comments · May be fixed by #10686
Assignees
Labels
bug Something isn't working devex developer experience

Comments

@mhofman
Copy link
Member

mhofman commented Nov 26, 2024

Describe the bug

Unlike promises, vows do not report unhandled rejections.

To Reproduce

watch a promise or vow providing a watcher that throws in onFulfilled. Do not install a watcher on the chained vow returned by watch.

Expected behavior

A console warning when a vow is rejected and it can no longer become watched.

Description of the Design

In vows

A new state field vowIsHandled in vows tracking if the vow was ever handled.
An optional vowRejectionTrackerHook(vow, state, reason) hook argument to prepareVowKit.

When shortening the vow, if the vow is not pending, record the vow as handled by setting state.vowIsHandled = true. If the vow was not previously handled and the vow is rejected, invoke vowRejectionTrackerHook?.(vow, 'handle')
When resolving/rejecting the vow, if there is an ephemeral kit, set state.vowIsHandled = true.
When rejecting the vow, if there is no ephemeral kit (state was not updated to handled), invoke vowRejectionTrackerHook?.(vow, 'reject', reason)

In vow tools

To reproduce the promise behavior of logging only on collection, we need to weakly attach a rejected promise to an unhandled rejected vow. Since we cannot durably store promises, we would need to use a (VOAware)WeakMap. Unfortunately that means we would miss diagnostics for vows rejecting when being unhandled in a first incarnation, but only being dropped in a later incarnation. To mitigate we can setup a watchPromise on a second unresolved promise weakly attached to the vow, where the rejection handler would report that a rejected vow was not handled before upgrade. Unfortunately it also means that if the vow is reported as unhandled in the first incarnation, we cannot cancel the reporting in the upgraded incarnation. This double reporting is inherent to the inability of userland to sense gc which unhandled rejections rely on.

On handling of the vow, the hook would handle the rejected promise and would fulfill the unresolved promise.

Security Considerations

None. Since we're dealing with userland helpers, they don't have any special privileges.

Scaling Considerations

This introduces quite a bit of bookkeeping in the case where an unhandled vow becomes rejected.

The reporting logic cannot retain a strong reference the vow since that would leak it and prevent reporting.

Additional heap state for reporting is acceptable: vows already consume heap state for their ephemeral kit.

Test Plan

Reproduction steps above combined with various timing of handling the vow, including after upgrade.

Upgrade Considerations

Introducing a new field in vow state is possible because no state shape is currently defined. However the logic of the behavior methods needs to account for a possibly missing field. For vows created before, the hooks would not be invoked, and the handling state not tracked (current behavior).

Reporting of unhandled rejected vows cannot be directly and reliably tied to garbage collection of these vows across upgrade. To avoid missing reports we prefer to eagerly report this case as a warning, even if that vow had already been reported as unhandled.

@mhofman mhofman added bug Something isn't working devex developer experience labels Nov 26, 2024
@mhofman mhofman self-assigned this Dec 9, 2024
@michaelfig michaelfig assigned michaelfig and unassigned mhofman Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working devex developer experience
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants