-
Notifications
You must be signed in to change notification settings - Fork 178
Add notification debouncing support to Protocol #407
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
base: main
Are you sure you want to change the base?
Conversation
094c294 to
474a21e
Compare
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.
Pull Request Overview
This PR updates the transport send method signature to support additional options and implements debounced notifications in the Protocol class, aligning with the MCP specification changes.
Key Changes:
- Added
TransportSendOptionsclass to encapsulate optional parameters for transport send operations (related request ID, resumption token, and resumption token callback) - Refactored
RequestOptionsto extendTransportSendOptionsand removedtimeoutfield fromProtocolOptions - Added
debouncedNotificationMethodstoProtocolOptionswith notification debouncing implementation - Updated all transport implementations to accept the new optional
optionsparameter
Reviewed Changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/TransportSendOptions.kt | New class defining options for transport send operations |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Transport.kt | Updated send method signature to include optional TransportSendOptions parameter |
| kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Protocol.kt | Refactored ProtocolOptions and RequestOptions, implemented notification debouncing logic |
| kotlin-sdk-core/api/kotlin-sdk-core.api | Updated binary API to reflect changes in Transport, ProtocolOptions, RequestOptions, and new TransportSendOptions class |
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/StdioServerTransport.kt | Updated send method signature to match Transport interface |
| kotlin-sdk-server/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/server/SSEServerTransport.kt | Updated send method signature to match Transport interface |
| kotlin-sdk-server/api/kotlin-sdk-server.api | Updated binary API for server transport implementations |
| kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StdioClientTransport.kt | Updated send method signature to match Transport interface |
| kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/SSEClientTransport.kt | Updated send method signature to match Transport interface |
| kotlin-sdk-client/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StreamableHttpClientTransport.kt | Updated to extract resumption token and callback from TransportSendOptions |
| kotlin-sdk-client/api/kotlin-sdk-client.api | Updated binary API for client transport implementations |
| kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/InMemoryTransport.kt | Updated test transport send method signature |
| kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/OldSchemaInMemoryTransport.kt | Updated test transport send method signature |
| kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/MockTransport.kt | Updated mock transport send method signature |
| kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/OldSchemaMockTransport.kt | Updated mock transport send method signature |
| kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/ClientTest.kt | Updated anonymous transport implementations in tests |
| kotlin-sdk-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/OldSchemaClientTest.kt | Updated anonymous transport implementations in tests |
| kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/KotlinServerForTsClientSse.kt | Updated HTTP server transport send method signature |
| kotlin-sdk-test/src/jvmTest/kotlin/io/modelcontextprotocol/kotlin/sdk/integration/typescript/sse/OldSchemaKotlinServerForTsClientSse.kt | Updated HTTP server transport send method signature |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| yield() | ||
| } finally { | ||
| _pendingDebouncedNotifications.update { it.remove(notification.method) } | ||
| } | ||
|
|
||
| val activeTransport = this@Protocol.transport ?: return@launch | ||
|
|
||
| try { | ||
| activeTransport.send(jsonRpcNotification, sendOptions) | ||
| } catch (cause: Throwable) { | ||
| logger.error(cause) { "Error sending debounced notification: ${notification.method}" } | ||
| onError(cause) |
Copilot
AI
Nov 19, 2025
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.
The debouncing logic has a potential race condition. The finally block removes the method from _pendingDebouncedNotifications (line 584) before the notification is actually sent (line 590). This means:
- After
yield()completes and thefinallyblock executes, the method is removed from the pending set - If another call to
notification()with the same method happens between the removal (line 584) and the send (line 590), it won't be blocked - This could result in multiple notifications being sent instead of being properly debounced
The removal should happen AFTER the send completes to maintain proper debouncing semantics. Consider restructuring the code like:
notificationScope.launch {
yield()
val activeTransport = this@Protocol.transport ?: run {
_pendingDebouncedNotifications.update { it.remove(notification.method) }
return@launch
}
try {
activeTransport.send(jsonRpcNotification, sendOptions)
} catch (cause: Throwable) {
logger.error(cause) { "Error sending debounced notification: ${notification.method}" }
onError(cause)
} finally {
_pendingDebouncedNotifications.update { it.remove(notification.method) }
}
}| try { | |
| yield() | |
| } finally { | |
| _pendingDebouncedNotifications.update { it.remove(notification.method) } | |
| } | |
| val activeTransport = this@Protocol.transport ?: return@launch | |
| try { | |
| activeTransport.send(jsonRpcNotification, sendOptions) | |
| } catch (cause: Throwable) { | |
| logger.error(cause) { "Error sending debounced notification: ${notification.method}" } | |
| onError(cause) | |
| yield() | |
| val activeTransport = this@Protocol.transport | |
| if (activeTransport == null) { | |
| _pendingDebouncedNotifications.update { it.remove(notification.method) } | |
| return@launch | |
| } | |
| try { | |
| activeTransport.send(jsonRpcNotification, sendOptions) | |
| } catch (cause: Throwable) { | |
| logger.error(cause) { "Error sending debounced notification: ${notification.method}" } | |
| onError(cause) | |
| } finally { | |
| _pendingDebouncedNotifications.update { it.remove(notification.method) } |
| public suspend fun start() | ||
|
|
||
| /** | ||
| * Sends a JSON-RPC message (request or response). |
Copilot
AI
Nov 19, 2025
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.
The documentation for the send method should describe the options parameter. Consider adding documentation like:
/**
* Sends a JSON-RPC message (request or response).
*
* @param message The JSON-RPC message to send
* @param options Optional transport-specific options for sending the message, such as
* related request ID for associating this message with an incoming request,
* or resumption token for continuing interrupted long-running requests
*/
public suspend fun send(message: JSONRPCMessage, options: TransportSendOptions? = null)| * Sends a JSON-RPC message (request or response). | |
| * Sends a JSON-RPC message (request or response). | |
| * | |
| * @param message The JSON-RPC message to send. | |
| * @param options Optional transport-specific options for sending the message, such as | |
| * related request ID for associating this message with an incoming request, | |
| * or resumption token for continuing interrupted long-running requests. |
kpavlov
left a comment
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.
To ensure source compatibility, we should define default values for all *Options parameters.
The current pattern of nullable *Options parameters is suboptimal because it necessitates null checks. To address this, we should make *Options parameters mandatory in all methods. This approach will enable the provision of mandatory configuration parameters, such as timeouts, and maintain logical simplicity. Adhering to a consistent pattern across all methods enhances consistency.
Is it indeed impossible to add tests for the change?
...ient/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/client/StdioClientTransport.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Protocol.kt
Outdated
Show resolved
Hide resolved
kotlin-sdk-core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/Transport.kt
Outdated
Show resolved
Hide resolved
...-test/src/commonTest/kotlin/io/modelcontextprotocol/kotlin/sdk/client/OldSchemaClientTest.kt
Outdated
Show resolved
Hide resolved
...core/src/commonMain/kotlin/io/modelcontextprotocol/kotlin/sdk/shared/TransportSendOptions.kt
Outdated
Show resolved
Hide resolved
|
|
||
| public open fun copy( | ||
| enforceStrictCapabilities: Boolean = this.enforceStrictCapabilities, | ||
| debouncedNotificationMethods: List<Method> = this.debouncedNotificationMethods, |
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.
I'm not sure we should implement debouncing (dropping some events) from the start/ever. The specification states that the implementation should support rate limiting, not debouncing.
I would remove the word debounced from the name, because it's very implementation-specific
| if (isDebounced) { | ||
| if (notification.method in _pendingDebouncedNotifications.value) { | ||
| logger.trace { "Skipping debounced notification: ${notification.method}" } | ||
| return | ||
| } | ||
|
|
||
| _pendingDebouncedNotifications.update { it.add(notification.method) } | ||
|
|
||
| notificationScope.launch { | ||
| try { | ||
| yield() | ||
| } finally { | ||
| _pendingDebouncedNotifications.update { it.remove(notification.method) } | ||
| } | ||
|
|
||
| val activeTransport = this@Protocol.transport ?: return@launch | ||
|
|
||
| try { | ||
| activeTransport.send(jsonRpcNotification, sendOptions) | ||
| } catch (cause: Throwable) { | ||
| logger.error(cause) { "Error sending debounced notification: ${notification.method}" } | ||
| onError(cause) | ||
| } | ||
| } |
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.
How were positive and negative scenarios tested? It seems worth separating into dedicated PR
474a21e to
e9700be
Compare
e9700be to
ab075b9
Compare
send and fix protocol notification
Motivation and Context
This change adds automatic debouncing for specified notification methods in the MCP protocol layer. This is needed to:
How Has This Been Tested?
Breaking Changes
None
Types of changes
Checklist