Skip to content

Close the session atomically with the executions - #163

Open
morisil wants to merge 1 commit into
issue-143-close-cancels-awaitfrom
issue-141-close-execute-race
Open

morisil wants to merge 1 commit into
issue-143-close-cancels-awaitfrom
issue-141-close-execute-race

Conversation

@morisil

@morisil morisil commented Nov 3, 2026

Copy link
Copy Markdown
Member

Fixes #141

Stacked on #161 (fix of #143, itself stacked on #158, #153 and #148) — the base of this PR is issue-143-close-cancels-await, so the diff shows only this fix.
Merge the PRs below it first; GitHub then retargets this PR to main.

Fix

Both races between close() and execute() are closed:

  • closed is an AtomicBoolean, and close() uses compareAndSet, so only one call cancels, queues the disposal and shuts the thread down — a concurrent second close() has no effect, instead of throwing RejectedExecutionException;
  • execute checks closed again on the session thread, inside withContext, before touching the compiler — so a snippet passing it was queued before the disposal, which always remains the last task on the thread;
  • asCoroutineDispatcher() is replaced by a small dispatcher which hands a rejected block to Dispatchers.IO without cancelling it, where its only effect is to fail that check with IllegalStateException("The session is closed")
    — before, a rejected snippet failed with CancellationException: The task was rejected, as if the caller was cancelled, and one queued after the disposal with IncorrectOperationException: … has already been disposed.

The KDoc of execute (@throws IllegalStateException, also for a session closed while the snippet waits for the ones running concurrently) and close() describe it.

DefaultScriptSession takes a new internal executor constructor parameter, a test seam to make the interleavings deterministic; the public ScriptSession(…) factory does not expose it, and the ABI dump is unchanged.

Tests

  • should reject snippet of session closed before dispatching it to the thread of the session — closes the session from within the dispatch, so it is rejected;
  • should reject snippet of session closed before dispatching it to the thread of the session after the disposal — the snippet, past the early check, is queued behind the disposal;
    both deterministic, failing before the fix (the reviewer re-verified it by removing the on-thread check);
  • should close session concurrently from many threads — a stress guard only: the race window is a single volatile read, so it did not fail before the fix either (see Harden the closing of a session: deterministic test, dispatcher fallback #162).

Review

One round of /code-review high, no blocking findings: the ordering of the CAS, the disposal and the shutdown was verified, and the custom dispatcher keeps what kotlinx.coroutines expects (isDispatchNeeded, the resumption on the caller's dispatcher).

Follow-ups filed

🤖 Generated with Claude Code

The closed check is repeated on the session thread, so a snippet dispatched
after close() fails with IllegalStateException instead of running on a disposed
compiler, or being cancelled by the rejecting dispatcher of asCoroutineDispatcher.
close() uses compareAndSet, so concurrent calls cannot submit to a shut down thread.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Nov 3, 2026

Copy link
Copy Markdown

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

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