Conversation
…ion (modelcontextprotocol#920) ServerSession.onInitialized composed callbacks into a plain var, so a callback registered after notifications/initialized was never invoked, and concurrent registration could drop callbacks. Keep pending callbacks in an AtomicRef<PersistentList?>. The initialized handler swaps it to null and runs the list in registration order; registration after that runs the callback immediately. Each callback now runs at most once. Fixes modelcontextprotocol#920
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #920
ServerSession.onInitializedput callbacks together in a plainvar, and thenotifications/initializedhandler invoked it. That caused two problems:notifications/initialized, it never runs.varis read and written without synchronization. WhenonInitializedis called from several threads while the handshake is in progress, two registrations can read the same old value, and one of the callbacks is then dropped.Pending callbacks are now stored in an
AtomicRef<PersistentList<() -> Unit>?>, using the same atomicfu + kotlinx-collections-immutable pattern asFeatureRegistry:onInitializedadds the callback withgetAndUpdate. If the ref was alreadynull, initialization has finished, so the callback runs right away on the caller's thread.getAndSet(null)and runs the list it gets back once, in registration order.Each callback now runs at most once, and callbacks pending when
notifications/initializedarrives run in registration order. (A callback never runs if the client doesn't sendnotifications/initialized, and if one throws, the ones after it don't run.) There is one behavior change: before, a duplicatenotifications/initializedran all callbacks again. Now it runs nothing. No public API changes (apiCheckpasses).This follows up #938, which targeted the same issue and was closed by its author.
Tests (in
ServerSessionInitializeTest):should run onInitialized callback registered after initialization: a callback registered after the handshake runs. Before the fix it failed with[early]instead of[early, late].should run every onInitialized callback registered concurrently: 200 callbacks are registered concurrently onDispatchers.Defaultbefore the handshake, and each must run once. Before the fix it failed in 5 out of 5 runs, with 2 to 16 callbacks dropped.I haven't run the full
integration-testsuite (the TypeScript interop tests) or the non-JVM targets.I used Claude Code to write this change and the PR description.