Set module internal promise rejection as handled when appropriate#534
Open
xim wants to merge 1 commit into
Open
Set module internal promise rejection as handled when appropriate#534xim wants to merge 1 commit into
xim wants to merge 1 commit into
Conversation
Before:
A module body is run as an async function. js_execute_sync_module calls
that function. It finds its result promise already rejected, reads the
result, and frees it without attaching a JS handler. The rejection fires
a unhandled notification. That notification is never balanced by a
is_handled=true notification. The internal promise rejection surfaces as
an unhandled rejection in addition to the module's evaluation promise.
Additional symptoms:
A dynamic import() whose rejection is fully handled by the caller still
leaked a unhandled rejection: the internal body promise stayed orphaned.
Without the fix a `import('m').then(ok, onerr)` of a throwing module
results in a unhandled rejection.
Fix:
Mark the consumed promise handled, emitting the balancing notification
with is_handled=true so the host can handle the rejection correctly.
With the fix, a `import('m').then(ok, onerr)` nets zero false positives.
Added a regression test in tests/test_std.js covering the dynamic import
case (this repo has no api-test harness).
Backported from quickjs-ng PR:
quickjs-ng/quickjs#1554
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
xim
force-pushed
the
module-rejection-handled
branch
from
48e2939 to
23792d2
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before:
A module body is run as an async function. js_execute_sync_module calls that function. It finds its result promise already rejected, reads the result, and frees it without attaching a JS handler. The rejection fires a unhandled notification. That notification is never balanced by a is_handled=true notification. The internal promise rejection surfaces as an unhandled rejection in addition to the module's evaluation promise.
Additional symptoms:
A dynamic import() whose rejection is fully handled by the caller still leaked a unhandled rejection: the internal body promise stayed orphaned. Without the fix a
import('m').then(ok, onerr)of a throwing module results in a unhandled rejection.Fix:
Mark the consumed promise handled, emitting the balancing notification with is_handled=true so the host can handle the rejection correctly.
With the fix, a
import('m').then(ok, onerr)nets zero false positives.Added a regression test in tests/test_std.js covering the dynamic import case (this repo has no api-test harness).
Backported from quickjs-ng PR:
quickjs-ng/quickjs#1554