-
Notifications
You must be signed in to change notification settings - Fork 14
Request cancellation for in-flight requests (ADR 0008) #30
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,9 +10,12 @@ module MCP.Server.Transport.Stdio | |
| ) where | ||
|
|
||
| import Control.Concurrent (ThreadId, forkIO, killThread) | ||
| import Control.Concurrent.MVar (modifyMVar_, newMVar, readMVar, | ||
| import Control.Concurrent.Async (Async, async, cancel) | ||
| import Control.Concurrent.MVar (modifyMVar_, newEmptyMVar, newMVar, | ||
| putMVar, readMVar, takeMVar, | ||
| withMVar) | ||
| import Control.Concurrent.STM (atomically, readTChan) | ||
| import Control.Exception (finally, uninterruptibleMask_) | ||
| import Control.Monad (forever, unless, when) | ||
| import Data.Aeson | ||
| import qualified Data.Aeson.KeyMap as KM | ||
|
|
@@ -61,23 +64,39 @@ transportRunStdio :: McpServerInfo -> McpServerHandlers -> IO () | |
| transportRunStdio = transportRunStdioWithConfig defaultStdioConfig | ||
|
|
||
| -- | Run the STDIO transport with the given configuration. | ||
| -- | ||
| -- Each request is served in its own task, so a @notifications/cancelled@ | ||
| -- naming its id can interrupt it mid-flight (after which nothing further is | ||
| -- written for that id). Cancellation reaches handler code as an | ||
| -- asynchronous exception: handlers that acquire resources should release | ||
| -- them with 'Control.Exception.bracket'. | ||
| -- | ||
| -- This also means requests run /concurrently/ (before 0.2.0.1 the stdio | ||
| -- transport processed them strictly sequentially): handlers touching | ||
| -- shared mutable state must synchronize, as was already required of | ||
| -- handlers used with the HTTP transport. | ||
| transportRunStdioWithConfig :: StdioConfig -> McpServerInfo -> McpServerHandlers -> IO () | ||
| transportRunStdioWithConfig config serverInfo handlers = do | ||
| -- Ensure UTF-8 encoding for all handles | ||
| hSetEncoding stderr utf8 | ||
| hSetEncoding stdout utf8 | ||
|
|
||
| -- Subscription threads and the main loop share stdout: one line at a time. | ||
| -- Subscription threads, request tasks and the main loop share stdout: | ||
| -- one line at a time. | ||
| writeLock <- newMVar () | ||
| -- Active subscriptions_listen streams, by their request id | ||
| subsVar <- newMVar ([] :: [(RequestId, ThreadId)]) | ||
| -- In-flight request tasks, by request id (cancellable) | ||
| inflightVar <- newMVar ([] :: [(RequestId, Async ())]) | ||
| -- Whether a legacy client has completed initialize (gates legacy pushes) | ||
| legacyReady <- newIORef False | ||
|
|
||
| let logLine = TIO.hPutStrLn stderr | ||
| logVerbose msg = when (stdioVerbose config) $ logLine msg | ||
|
|
||
| sendRaw bytes = withMVar writeLock $ \_ -> do | ||
| -- Writes are uninterruptible so a cancellation arriving mid-write | ||
| -- cannot corrupt the message channel with a half line | ||
| sendRaw bytes = withMVar writeLock $ \_ -> uninterruptibleMask_ $ do | ||
| TIO.putStrLn $ TE.decodeUtf8 $ BSL.toStrict bytes | ||
| hFlush stdout | ||
| sendMessage msg = sendRaw $ encode $ encodeJsonRpcMessage msg | ||
|
|
@@ -136,6 +155,28 @@ transportRunStdioWithConfig config serverInfo handlers = do | |
| sendResponse $ closureResponse serverInfo subId) | ||
| subs | ||
|
|
||
| -- Cancel an in-flight request task. 'cancel' waits for the task to | ||
| -- finish, so once this returns nothing further is written for that id. | ||
| cancelInflight cancelledId = do | ||
| inflight <- readMVar inflightVar | ||
| case lookup cancelledId inflight of | ||
| Nothing -> pure False | ||
| Just task -> do | ||
| cancel task | ||
| logLine $ "Cancelled request " <> T.pack (show cancelledId) | ||
| pure True | ||
|
|
||
| dispatchMessage message = do | ||
| -- Request-scoped notifications (progress, client logs) share the | ||
| -- locked stdout channel, interleaved before the response | ||
| response <- handleMcpMessage serverInfo (stdioCacheHints config) notifSupport sendNotification handlers anonymousContext message | ||
| case response of | ||
| Just responseMsg -> do | ||
| logLine $ "Sending response for: " <> T.pack (show (getMessageSummary message)) | ||
| sendMessage responseMsg | ||
| Nothing -> | ||
| logLine $ "No response needed for: " <> T.pack (show (getMessageSummary message)) | ||
|
|
||
| handleParsed message = case message of | ||
| -- subscriptions/listen is transport-level: the stream outlives the | ||
| -- request. Only intercept when a source is configured and the | ||
|
|
@@ -147,15 +188,33 @@ transportRunStdioWithConfig config serverInfo handlers = do | |
| , maybe True (`elem` modernVersions) (metaProtocolVersion (requestParams req)) | ||
| -> openSubscription src req | ||
|
|
||
| -- notifications/cancelled referencing an open subscription tears it | ||
| -- down (no response, per the cancellation rules) | ||
| -- Every other request runs in its own task so a later | ||
| -- notifications/cancelled can interrupt it while the read loop keeps | ||
| -- serving. The task is registered before its body starts (the gate) | ||
| -- so cancellation can never race registration; it deregisters itself | ||
| -- on any exit, including cancellation. | ||
| | otherwise -> do | ||
|
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Requested fix 1: document that stdio handlers now run concurrently. This branch changes a real contract: before this PR the stdio transport processed requests strictly sequentially, so handlers with shared mutable state (an
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in 6cc8355 — README (new paragraph in the Cancellation section), the stdio transport haddock, and the CHANGELOG (flagged as a behavioral break under 0.2.0.1) now all state that requests are served concurrently on both transports and that handlers touching shared mutable state must synchronize. |
||
| let rid = requestId req | ||
| gate <- newEmptyMVar | ||
| task <- async $ do | ||
| takeMVar gate | ||
| dispatchMessage message | ||
| `finally` modifyMVar_ inflightVar (pure . filter ((/= rid) . fst)) | ||
| modifyMVar_ inflightVar (pure . ((rid, task) :)) | ||
| putMVar gate () | ||
|
|
||
| -- notifications/cancelled tears down the referenced subscription or | ||
| -- in-flight request (no response either way, per the cancellation | ||
| -- rules; unknown ids are ignored as the spec requires) | ||
| JsonRpcMessageNotification notif | ||
| | notificationMethod notif == "notifications/cancelled" | ||
| , Just cancelledId <- cancelledRequestId (notificationParams notif) | ||
| -> do | ||
| wasSub <- cancelSubscription cancelledId | ||
| unless wasSub $ | ||
| logLine $ "Ignoring cancellation for unknown request " <> T.pack (show cancelledId) | ||
| unless wasSub $ do | ||
| wasInflight <- cancelInflight cancelledId | ||
| unless wasInflight $ | ||
| logLine $ "Ignoring cancellation for unknown request " <> T.pack (show cancelledId) | ||
|
|
||
| _ -> do | ||
| -- The client's initialized notification is the legacy ready signal: | ||
|
|
@@ -167,20 +226,14 @@ transportRunStdioWithConfig config serverInfo handlers = do | |
| | notificationMethod n == "notifications/initialized" -> | ||
| writeIORef legacyReady True | ||
| _ -> pure () | ||
| -- Request-scoped notifications (progress, client logs) share the | ||
| -- locked stdout channel, interleaved before the response | ||
| response <- handleMcpMessage serverInfo (stdioCacheHints config) notifSupport sendNotification handlers anonymousContext message | ||
| case response of | ||
| Just responseMsg -> do | ||
| logLine $ "Sending response for: " <> T.pack (show (getMessageSummary message)) | ||
| sendMessage responseMsg | ||
| Nothing -> | ||
| logLine $ "No response needed for: " <> T.pack (show (getMessageSummary message)) | ||
| dispatchMessage message | ||
|
|
||
| loop = do | ||
| eof <- hIsEOF stdin | ||
| if eof | ||
| then do | ||
| inflight <- readMVar inflightVar | ||
| mapM_ (cancel . snd) inflight | ||
| closeAllSubscriptions | ||
| logLine "stdin closed - shutting down" | ||
| else do | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested fix 2: the single-JSON disconnect claim is stronger than Warp's actual behavior. A WAI application doesn't learn the client is gone until it writes — Warp doesn't poll the socket while the handler runs. For streaming responses your keep-alive writer is precisely what surfaces the disconnect; for single-JSON responses there is no mid-handler write, so an abandoned request runs the handler to completion and only then fails the response write. The thread does get torn down — but after the work is done, which is not "cancellation" in the spec's SHOULD-stop-work sense. Suggest rewording here, in the README, and in the CHANGELOG fragment to something like: "for single-JSON responses a disconnect is only detected at the final write, so mid-handler cancellation applies to streaming (SSE) requests — clients wanting cancellable calls should opt into streaming via a progressToken." That last clause is genuinely useful guidance for client authors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 6cc8355 — reworded in the HTTP haddock, README and CHANGELOG: single-JSON responses only detect a disconnect at the final write (handler runs to completion first), so mid-handler cancellation applies to streaming requests, with the suggested guidance that clients wanting cancellable calls opt into streaming via a progressToken.