refactor: make CancelToken Promise-based#13303
Conversation
Move CancelToken from Init.System.IO to its own file Init.System.CancelToken, backed by IO.Promise Unit instead of IO.Ref Bool. This adds a CancelToken.task method returning a Task Unit that completes when cancelled, enabling non-polling cancellation propagation via IO.mapTask. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Mathlib CI status (docs):
|
|
Reference manual CI status:
|
This PR updates `check_interrupted()` in the C++ runtime to check the Promise-based `CancelToken` directly via `lean_io_get_task_state_core`, rather than calling the Lean-exported `lean_io_cancel_token_is_set`. The old approach failed during stage2 build because the stage0-compiled export assumed the IORef-based representation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Update `delabConst.lean` and `printStructure.lean` to reflect that `IO.CancelToken` now wraps `IO.Promise Unit` instead of `IO.Ref Bool`. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add `LEAN_TASK_STATE_{WAITING,RUNNING,FINISHED}` constants in `lean.h`
matching `IO.TaskState`, and an `lean::promise_is_resolved` inline in
`object.h`. Replace the open-coded `lean_io_get_task_state_core(...) == 2`
checks in `interrupt.cpp`, `uv/timer.cpp`, and `uv/signal.cpp` with this
helper.
Also drop the manual `inc_ref(g_cancel_tk)` in `check_interrupted`: the
token is owned by the enclosing `scope_cancel_tk`, so it's alive for the
duration of the call. Document this with a comment.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Add `onSet` for registering a callback to run when the token is set. Internally uses `BaseIO.chainTask` on the promise's `result?` task directly; this avoids the fresh `Task.map` produced by `Promise.result!` that `task` exposes, which is a footgun when chaining further dependencies (intermediate task gets GC'd, dep chain silently breaks). Document the hazard on `task`. Document that `new` cannot be called from `initialize` blocks (task manager not running yet) and recommend lazy construction. Fix a race in `getUnblockedCancelTk` (test helper): previously two callers could each create a fresh token with only one stored; use `modifyGet` for an atomic check-or-install. Add `tests/elab/cancel_token.lean` exercising `onSet` (before/after set, multiple callbacks) and `task`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Switch from `tk.promise.result!` (which wraps `result?` in a fresh `Task.map` per call, and panics if the promise is dropped without being set) to `tk.promise.result?` directly. The signature changes to `Task (Option Unit)`. The returned task is now stable across calls — same task object every time — so chaining further dependencies (`Task.map`, `BaseIO.bindTask`) on it is safe. The `Option` distinguishes a normal `set` (`some ()`) from the token being dropped without ever being set (`none`). For attaching a callback when the token is set, prefer `onSet`. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
In compiled binaries (and `lean --run`), the task manager is initialized *after* `io_mark_end_initialization`, so `initialize` blocks see `g_task_manager == nullptr`. Previously this surfaced as a bare `LEAN ASSERTION VIOLATION ... Condition: g_task_manager` with no clue about the cause. Replace the assert with `lean_internal_panic` carrying a message that names `Promise.new`, mentions the typical trigger (`initialize` / `IO.CancelToken.new`), and recommends lazy construction. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
!radar |
|
Benchmark results for a5ae5cb against d48863f are in. There are significant results. @nomeata Warning These warnings may indicate that the benchmark results are not directly comparable, for example due to changes in the runner configuration or hardware.
Large changes (1🟥)
Medium changes (6🟥)
Small changes (324🟥) Too many entries to display here. View the full report on radar instead. |
…lean4 into joachim/cancellation-promise
|
!radar |
|
Benchmark results for 633962f against 3c6317b are in. There are significant results. @nomeata
Large changes (2🟥)
Medium changes (5🟥)
Small changes (344🟥) Too many entries to display here. View the full report on radar instead. |
mhuisi
left a comment
There was a problem hiding this comment.
Looks OK to me.
- Creating cancellation tokens in initializers is hopefully nothing anyone needs to do?
- I wonder if we can somehow avoid the +0.4% overhead?
| The `Option` distinguishes a normal `set` (`some ()`) from the token being dropped without ever | ||
| being set (`none`). For attaching a callback, prefer `onSet`. | ||
| -/ | ||
| def task (tk : CancelToken) : Task (Option Unit) := |
There was a problem hiding this comment.
Can this be Task Bool (via a sync map)?
There was a problem hiding this comment.
We had something like that before on this branch (via Promise.result! which does a sync map) and it was causing issues (either things being GC’ed or just the extra cost of more Tasks).
If you think that it should not have caused these issues and it was likely something else I can look again.
There was a problem hiding this comment.
Note that Promise.result! uses Option.getOrBlock!, which is almost certainly not what you want here, just the sync map.
I do think that we should get this API right now instead of having to break it again in the future, yes.
Henrik already alerted me and suggested I follow the |
Add an `IO.Ref Bool` field to `CancelToken` that's set in lockstep with the promise resolution. `CancelToken.isSet` reads the flag directly, and the C++ `check_interrupted` hot path uses a small inline accessor that projects the flag and reads it without walking the promise's task state. The intent is to recover the ~0.4% performance regression that the Promise-based representation introduced on `Core.checkInterrupted`-heavy workloads, by replacing a task-state walk with a direct ref read. Not benchmarked yet — the optimization is plausible but unmeasured. The same dual-state pattern is already used in `Lean.Server.RequestCancellationToken` (CancelToken + separate Promise); folding it into `CancelToken` itself lets us simplify that downstream. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
!radar |
|
Benchmark results for a8edb0e against 3c6317b are in. There are significant results. @nomeata
Medium changes (2✅, 1🟥)
Small changes (47✅, 1🟥) Too many entries to display here. View the full report on radar instead. |
| A task that completes when the cancellation token is set or dropped. Useful for waiting on | ||
| cancellation without polling — e.g. as one of the tasks given to `IO.waitAny`. |
There was a problem hiding this comment.
This is strictly speaking not true anymore. Consumers can now occasionally observe that tk.isSet is true while tk.task has not completed yet.
This isn't a big deal in the language server since it's all internal API, but I don't think it's impossible that some code calls e.g. isSet to determine whether the cancellation token has been set and then (carelessly) calls some other downstream function they don't control that uses something akin to SnapshotTask.get?, assuming that it will work, when in reality they have now introduced a super subtle race condition.
This PR moves
IO.CancelTokenfromInit.System.IOto its own fileInit.System.CancelToken, backed byIO.Promise Unitinstead ofIO.Ref Bool. This enables non-polling cancellation propagation: the token's underlying promise can be used directly withIO.waitAny, and callbacks can be registered to fire when cancellation is requested.The structure carries both the promise and a plain
IO.Ref Boolflag, set in lockstep byset.isSetreads the flag directly (used on hot paths likeCore.checkInterrupted);task/onSetgo through the promise. The avoids a ~0.4% regression that a pure-promise representation introduced.API additions:
CancelToken.task : Task (Option Unit). Returns the underlying promise'sresult?task directly — the same task object on every call, so furtherTask.map/BaseIO.bindTaskdependencies can be safely attached. Resolves withsome ()whensetis called, ornoneif the token is dropped without ever being set.CancelToken.onSet : BaseIO Unit → BaseIO Unit. Registers a callback that runs synchronously on the cancelling thread whensetis called (or immediately if the token is already set). Implemented viaBaseIO.chainTaskonresult?, so no freshTask.mapper call and no GC hazard.Runtime cleanup:
LEAN_TASK_STATE_{WAITING,RUNNING,FINISHED}constants inlean.hmatchingIO.TaskState.lean::promise_is_resolvedinline inobject.h, replacing three open-codedlean_io_get_task_state_core(...) == 2checks (ininterrupt.cpp,uv/timer.cpp,uv/signal.cpp).inc_ref(g_cancel_tk)incheck_interrupted; the token is owned by the enclosingscope_cancel_tkfor the duration of the call (documented).lean_always_assert(g_task_manager)inlean_promise_newwith an explicitlean_internal_paniccarrying a message that namesPromise.new, identifies the typical trigger (initializeblocks, transitively viaIO.CancelToken.new), and recommends lazy construction. Without this, users got an opaque "LEAN ASSERTION VIOLATION ... Condition: g_task_manager" with no actionable hint.Behavioural notes documented inline:
newcannot be called frominitializeblocks (task manager not running yet); construct lazily.taskdocuments the dropped-promise case (none) and steers callers toonSetfor callback chaining.A consumer of
onSetfor parent → child cancel-token propagation in parallel tactic combinators is in #13428 (fixes #13300).Co-Authored-By: Claude Opus 4.7 noreply@anthropic.com