| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
Model not found: cloudflare-ai-gateway/anthropic/claude-opus-4-6. Did you mean: anthropic/claude-opus-4.5, anthropic/claude-opus-4.6, anthropic/claude-opus-4.7? |
Sorry, something went wrong.
|
@jasnell Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
Sorry, something went wrong.
Merging this PR will improve performance by 9.65%⚠️ Different runtime environments detected
⚡ 1 improved benchmark Performance Changes
Tip Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent. Comparing jasnell/eventtarget-abortsignal-improvements (45e2c5c) with main (5ef8a1f)2 Footnotes
|
Sorry, something went wrong.
|
@cloudflare/wrangler folks... the failing workers-sdk test... it's unclear if that's caused by this change set or ... something else. |
Sorry, something went wrong.
|
/bonk give this a good review |
Sorry, something went wrong.
|
Posted 2 actionable inline findings. No test suite was run. Time for a pun! This review found a couple of signals that should not be ignored. |
Sorry, something went wrong.
|
Compat flag doc and changelog PR: cloudflare/cloudflare-docs#32863 |
Sorry, something went wrong.
|
/bonk re-review |
Sorry, something went wrong.
|
LGTM! |
Sorry, something went wrong.
|
(nvm, DO actor shares one long-lived IOcontext, I was incorrect) |
Sorry, something went wrong.
There was a problem hiding this comment.
The core rework here is solid — the registration-cell model, cross-context delivery, dependent-signal handling, GC tracing, and the dispatch snapshotting all check out, and the earlier bot findings are properly addressed. Main concern is a pair of flag-off regressions where call sites dropped their legacy catch handlers unconditionally while the gating only protects the dispatch semantics (inline comments 1-2); the rest are smaller.
Findings:
Note: this review was performed with AI assistance under my direction.
Sorry, something went wrong.
There was a problem hiding this comment.
All findings from my previous review are addressed: the flag-off legacy exception routing is restored for EventSource and MessagePort (with the new pinned-off legacy-dispatch-exceptions-test covering both), the WebSocket CloseEvent dispatches are report-only with PROPAGATE fall-through preserving the old flag-off behavior, and the trusted initializer and addAbortAction doc are fixed.
One new finding on the triggerAbort dedup fix (inline): folding setAbortState() into the collection loop opens a JS-reentrancy window that can mutate dependentSignals mid-iteration. Worth fixing, but it requires a poisoned getter on the abort reason to reach, so approving — fine as a fast-follow if preferred.
Note: this review was performed with AI assistance under my direction.
Sorry, something went wrong.
Improve the implementation. We don't need this to be Refcounted with the additional changes being made to AbortSignal in subsequent commits. Also, clean up the implementation a bit and prepare AbortableImpl for the AbortSignal refactor.
Re-use the same signaling mechanism used by cross-request promise resolution for cross-request abort signaling.
Allows an AbortSignal/AbortController to be created outside of an IoContext. The IoContext dependency will be captured by individual abort algorithms attached when necessary. This makes it possible for a single AbortSignal to trigger *across* requests safely and soundly.
Make the implementation more spec compliant. The changes in behavior are not flagged as it is highly unlikely for anyone to be depending on the odd non-standardized event ordering in the original. Additional tests are added to strengthen coverage
Event's constructor defaulted trusted to Trusted::YES, so any subclass constructor that did not explicitly pass Trusted::NO produced events reporting isTrusted === true even when constructed from JavaScript. Flip the default to Trusted::NO so untrusted is the safe default, and make runtime-constructed events opt in explicitly: No compat flag: isTrusted was already false for plain Events and reflects trusted correctly for runtime events. It's exceedingly unlikely that anyone is depending on the broken, non-standard behavior.
Ensures that all event handlers for a given event are run, even if one throws. Rather than throwing synchronously, the error is dispatched to the `'error'` event on the global scope. Because these are runtime dispatches, it is unlikely to break user code... or at least, it should be extremely unlikely too. The existing behavior is non-standard and unexpected by most code.
- `listenerCountChanged` override replacing the dead `addEventListener` shadow - cross-context arming routing with tests and doc - `reportError` stack-getter guard + events test
* Restore the legacy exception routing on the flag-off dispatch paths. With spec_compliant_dispatch_exceptions disabled, EventSource message dispatch routes a throwing listener back through notifyError() (error event + close) instead of failing the enclosing read-loop task, and MessagePort message dispatch restores the swallow-and-redispatch behavior (a second 'message' event carrying the exception) instead of producing an unhandled delivery-microtask rejection. The 'messageerror' event type remains gated behind the flag. Adds legacy-dispatch-exceptions-test with the disable flag pinned so every variant exercises the flag-off path. * Dispatch WebSocket CloseEvents report-only. The socket is already closed or failed when 'close' fires, so rethrowing the first listener exception only re-surfaced an already-reported exception into terminal plumbing and skipped the cleanup following the dispatch. 'open' and 'message' keep the fail-fast rethrow, which feeds a real reaction. * Set the abort state of dependent signals as they are collected in triggerAbort() so that a signal linked more than once (e.g. any([s, s])) has its abort steps run only once, rather than relying on the second run being accidentally inert. * Flip Event's dead trusted bitfield initializer to match the untrusted-by-default invariant, and document that dropping a native abort registration handle destroys the callback's captures on the dropping thread.
There was a problem hiding this comment.
I mostly just checked the d.ts files, in any case LGTM 🙂
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Larger PR split intentionally into smaller incremental commits for easier review. I recommend first looking at the tests and documentation changes to get a big picture view. Then, walk through the commits one at a time to review in batches.
Long standing todo here that has been put off long enough.
AbortSignal
scope and created/observed/aborted across different requests. new AbortController(),
AbortSignal.abort(), new WritableStream(), and request.signal no longer throw
during module evaluation. AbortSignal.timeout() still requires an IoContext to schedule
the timeout.
gone. Each wrap() / newCanceler() / addAbortAction() registers a cell bound to the
registering request's IoContext. Aborts triggered elsewhere are delivered into the owning
context on its next turn (or silently dropped if it's gone); registrations are reclaimed on
completion and at IoContext teardown, and swept thereafter, so long-lived signals don't
accumulate per-request state.
than cancels wrapped promises on drop, unlinks listeners as it fires them, and fires
late-registered listeners immediately.
(runs before abort listeners); AbortSignal.any() uses the spec's dependent-signals model
with flattening (fixes the expected-fail WPT ordering test; deletes the followingSignal and
synthesized-listener hacks); triggerAbort follows the spec's "signal abort" sequence;
synthetic dispatchEvent('abort') no longer runs internal plumbing; pre-aborted wrap()
rejects with the reason-derived exception instead of throwing a TypeError.
its activation instead of always firing first.
requests); the pending abort reason arrives in a mutex-guarded box readable from any
context; arming the abort subscription is centralized in addAbortAction() and gated to
abort-relevant registrations (no longer blocks actor hibernation for unrelated listeners).
EventTarget
bidirectional bare-reference lifetimes, custom destructor, and the GC-visitation special
case). EventHandler is a flat, identity-keyed record; internal consumers use the
AbortSignal primitives above instead.
report listener exceptions (via the global scope's error event) and continue, per spec —
so abort() can no longer throw from a throwing listener. The runtime's top-level event
delivery (fetch/scheduled/etc.) keeps the propagate behavior.
implementing positioned event-handler attributes.
New internal API surface: AbortSignal::wrap() (rejects with the abort reason),
newCanceler(), addAbortAction() (context-bound), addAbortAlgorithm() (JS-heap);
documented in docs/reference/detail/abort-signal.md.
Additional edits
YES, so user-constructed subclasses (CustomEvent, MessageEvent, ErrorEvent,
CloseEvent) incorrectly reported isTrusted === true. Untrusted is now the default;
runtime construction sites (and runtime-only subclass constructors such as
ExtendableEvent) pass Trusted::YES explicitly.
EventTarget::get/setEventHandlerAttribute() implements HTML's event handler IDL
attribute semantics once (trampoline listener at first-assignment position, kept across
reassignment, fresh position after clearing, non-callable objects stored but never
invoked). AbortSignal.onabort migrates onto it (deleting its bespoke copy);
MessagePort.onmessage and EventSource.on{open,message,error} no longer always fire
before addEventListener() listeners.
is replaced with a listenerCountChanged() virtual notified on every listener-set
mutation. Fixes addEventListener('message') never starting a MessagePort (only
onmessage assignment did), and makes a closed port terminal (clearing onmessage
could previously reset it to pending).
MessagePort runtime-fired events now dispatch with spec semantics: a throwing listener is
reported (global error event + console) and the remaining listeners still run.
The historical fail-fast reaction is preserved on top via
DispatchResult::firstException: the WebSocket still errors out, the EventSource still
closes (dropping the rest of the batch), and the MessagePort still dispatches
messageerror. Also fixes reportError() re-entrancy: a throwing global error
listener now logs to console (HTML's "in error reporting mode" flag) instead of
propagating out of the dispatch or recursing.
spec surface (data now optional defaulting null, plus origin, lastEventId,
source, ports, and the EventInit members); CloseEventInit/ErrorEventInit gain
bubbles/cancelable/composed. Runtime-constructed events are unchanged.
Behavioral changes
registering any 'message' listener starts a MessagePort.
exception surfaces on the global error event/console in addition to the existing
per-object failure reaction. A throwing 'message' listener on a MessagePort now
triggers messageerror (previously a second 'message' event carrying the exception).
Compat flags?
The behavioral changes here should be unlikely to require compatibility flags, but I could be convinced with some good practical examples.