Skip to content

Make promise task outcome task-local in processNextTick - #173

Open
yuxi-liu-wired wants to merge 1 commit into
funcool:masterfrom
yuxi-liu-wired:fix/task-local-promise-state
Open

yuxi-liu-wired wants to merge 1 commit into
funcool:masterfrom
yuxi-liu-wired:fix/task-local-promise-state

Conversation

@yuxi-liu-wired

Copy link
Copy Markdown

processNextTick kept state across tasks of one queue. So a task that threw left every later task REJECTED, and a reject handler that returned a value never set it back to RESOLVED. (p/resolved nil) returns one shared NULL_PROMISE, so independent chains from nil share a queue and one chain's exception corrupts the others.

Reproduction

const a = P.resolved(null).then(() => { throw new Error("A"); });
const b = P.resolved(null).then(() => "b-ok");
a.catch(() => null);

const c = P.resolved(null).then(() => { throw new Error("C"); })
  .catch(e => "recovered:" + e.message).then(v => v + ":continued");
const d = P.resolved(null).then(() => "d-ok");
11.0.678 master (before) this PR (after)
b rejected A resolved null "b-ok"
c "recovered:C:continued" rejected "recovered:C" "recovered:C:continued"
d rejected C resolved null "d-ok"

Fix

  • state/rvalue are task-local. Each outcome depends only on the source promise and the task's own handler: return means RESOLVED (a reject handler that returns is a recovery), throw means REJECTED.
  • handle uses defaultRejectHandler again (as in 11.0.678). Its identity reject handler only worked because the loop-level state leaked.
  • A task whose reject handler is defaultRejectHandler gets the rethrow outcome directly, with no throw. This avoids a ~2.5× slowdown in handle on rejected promises. Benchmarks (1M ops, 15 interleaved rounds): resolved and rejected paths match master within noise, and rejection propagation through .then/.catch is ~2× faster (655 → 314 ms).

Tests

New :cljs tests in core_test.cljc: independent-chains-from-nil-do-not-share-outcome and throw-in-then-recover-in-catch-and-continue. Both fail on master (3 assertions) and pass with the fix.

  • CLJS: 82 tests, 281 assertions, 0 failures (ClojureScript 1.12.42 via cljs.main, since Clojars was unreachable from the environment the change was made in; the CI job runs the shadow-cljs build).
  • JVM (all promesa.tests.*-test): 92 tests, 263 assertions, 0 failures.
  • bb (bb test:bb): 68 tests, 160 assertions, 0 failures.

🤖 Generated with Claude Code

The drain loop kept `state` across tasks of the same queue, so a task
that threw left `state` as REJECTED for every following task, and a
reject handler that returned a value never switched it back to
RESOLVED. Since (p/resolved nil) returns a shared NULL_PROMISE,
independent chains started from nil share one queue, and an exception
in one chain corrupted the result of the others.

Each task's state/value is now computed only from the source promise
and its own handler. A reject handler that returns normally recovers
(RESOLVED); one that throws rejects. `handle` goes back to using
defaultRejectHandler (as in 11.0.678) so rejections still reach its
completion callback as the cause.

To avoid paying for a throw on every propagated rejection, a task whose
reject handler is defaultRejectHandler gets the same outcome the
rethrow would produce, without calling it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019iotb3eKMwNTZTnsonJb3F

This branch has not been deployed

No deployments
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.

1 participant