Promise and Job Queue API Completion#751
Conversation
- add runtime job queue APIs for pending checks, pending context lookup, and executing one pending job - add context promise primitives including PromiseCapability, NewPromiseCapability, NewSettledPromise, and EnqueueJob alias - add focused runtime/context tests for normal and fail-closed paths while keeping full coverage green
- add runtime APIs for promise hook and host promise rejection tracker with owner/liveness fail-closed behavior - bridge quickjs promise hook and host rejection tracker callbacks from C into Go runtime dispatch - add runtime tests for callback lifecycle, fail-closed behavior, and dispatch guard paths while preserving full coverage
- align Value.PromiseState with quickjs-ng semantics by returning PromiseNotAPromise for non-promise values - add Value.PromiseResult to expose native JS_PromiseResult behavior for fulfilled, rejected, pending, and non-promise values - update context/value tests to cover the new semantics and PromiseResult API while keeping full coverage at 100%
- add Runtime.DrainPendingJobs(max) to drain pending jobs with optional execution limit - propagate job execution errors with context exception fallback semantics - add runtime tests for limit, full drain, error path, and fail-closed behavior while preserving full coverage
- add Context.EnqueueNativeJob to enqueue callable jobs through QuickJS native JS_EnqueueJob - introduce bridge helper EnqueueCallableJob with callable validation and JS call dispatch in native job callback - add context promise primitive tests for enqueue success, thrown job errors, and fail-closed guards while keeping full coverage
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #751 +/- ##
===========================================
- Coverage 100.00% 99.94% -0.06%
===========================================
Files 17 17
Lines 3831 3989 +158
===========================================
+ Hits 3831 3987 +156
- Misses 0 2 +2
Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for promise hooks, host promise rejection tracking, and enhanced job queue management in the QuickJS Go bindings. Key additions include NewPromiseCapability and NewSettledPromise for advanced promise control, as well as DrainPendingJobs and EnqueueNativeJob for finer execution scheduling. Review feedback suggests refining the job execution counter in DrainPendingJobs to ensure it accurately reflects the number of tasks processed and adding explicit exception checks when creating promise capabilities to prevent the use of invalid handles.
- count DrainPendingJobs executions per processed job iteration for clearer semantics - return nil from NewPromiseCapability when JS_NewPromiseCapability reports an exception - add a focused low-memory regression test to cover the capability exception path while preserving 100.0% total coverage
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly expands the promise and job queue management capabilities of the library. It introduces support for runtime-level promise hooks, host promise rejection tracking, and the ability to enqueue native jobs directly into the QuickJS queue. New API methods such as EnqueueNativeJob, NewPromiseCapability, and DrainPendingJobs have been added, along with corresponding C bridge implementations and comprehensive unit tests. The review feedback highlights an opportunity to optimize memory allocations in EnqueueNativeJob to reduce GC pressure and suggests adding safeguards or documentation regarding potential infinite loops when draining the job queue without a limit.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for promise lifecycle hooks, host rejection tracking, and native job enqueuing. Key additions include EnqueueNativeJob, NewPromiseCapability, and NewSettledPromise in the Context type, alongside job queue management methods such as DrainPendingJobs in the Runtime type. Feedback identifies potential issues with context resolution during exceptions in ExecutePendingJob, suggests earlier validation for callable jobs in EnqueueCallableJob, and recommends ensuring no reference leaks occur in NewPromiseCapability upon failure. Additionally, it is noted that DrainPendingJobs side-effects the context by clearing exceptions, which should be documented.
| status := int(C.JS_ExecutePendingJob(r.ref, &ctxRef)) | ||
| if ctxRef == nil { | ||
| return status, nil | ||
| } | ||
| return status, getContextFromJS(ctxRef) |
There was a problem hiding this comment.
In ExecutePendingJob, if status < 0 (indicating an exception), the code attempts to resolve the Context via getContextFromJS(ctxRef). If the context is not found in the internal mapping (e.g., due to a race condition during shutdown or an external context creation), it returns nil for the context. This makes it impossible for the caller to retrieve or clear the exception, potentially leaving the runtime in an inconsistent state. Consider adding a fallback or a warning if the context cannot be resolved when an exception is reported.
| if (!ctx || argc < 1 || !argv) { | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
The EnqueueCallableJob function does not validate if argv[0] is actually a function before enqueuing. While the job function QuickjsGoCallableJob performs this check during execution, enqueuing a non-callable value will lead to a TypeError being raised asynchronously in the job queue. It is better to validate this early to provide immediate feedback to the caller.
| if bool(C.JS_IsException(promise)) { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
When JS_NewPromiseCapability fails and returns JS_EXCEPTION, the resolving array might contain partially initialized JSValue objects if the engine failed after creating one of the functions but before completing the capability. Although QuickJS usually handles internal cleanup, it is safer to explicitly check for exceptions and ensure no leaked references remain in the resolving array if the call fails.
| err = errors.New("quickjs: failed to execute pending job") | ||
| if ctx != nil && ctx.HasException() { | ||
| err = ctx.Exception() | ||
| } |
There was a problem hiding this comment.
In DrainPendingJobs, when an error occurs (status < 0), the code calls ctx.Exception() which clears the exception from the QuickJS context. While this is necessary to convert the exception to a Go error, it means that subsequent calls to DrainPendingJobs or other error-checking methods on the same context will not see the original exception. This behavior should be clearly documented as it side-effects the state of the Context.
1 participant
promise and job-queue surface with a consistent Go-facing API aligned to quickjs-ng behavior.
It adds practical runtime control for pending job processing and native callable job enqueueing, and it aligns promise semantics for state and result handling, while preserving existing Await and scheduler behavior. All new entry points follow the same fail-closed safety model for invalid lifecycle or ownership conditions.
The changes were delivered incrementally with focused positive and negative-path tests for execution limits, enqueue validation, and error propagation across the Go/C boundary. The full test suite passes, and total coverage remains at 100.0%.