Aias00 commented on code in PR #6997:
URL: https://github.com/apache/shenyu/pull/6997#discussion_r4109984014
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/test/java/org/apache/shenyu/plugin/mcp/server/transport/ShenyuStreamableHttpServerTransportProviderTest.java:
##########
@@ -204,6 +204,37 @@ void testStaleSessionRestoreCleansUpCreatedSession()
throws Exception {
assertEquals(0, readMap(provider, "sessionTransports").size());
}
+ /**
+ * Regression test for the reported initialize-path session leak (#6833).
+ *
+ * <p>Before the fix the provider stored the session under the MCP server
session ID while the
+ * transport kept its own independently auto-generated {@code sessionId},
so {@code close()}
+ * looked up a key that never existed in {@code sessions} / {@code
sessionTransports} and left
+ * both registries plus {@link ShenyuMcpExchangeHolder} populated forever.
+ *
+ * <p>Assertion: after a real initialize handshake the ID returned to the
client
+ * ({@code Mcp-Session-Id}) is exactly the key used in both registries, so
a later
+ * {@code close()} is able to remove the entry.
+ */
+ @Test
+ void testInitializeRegistersSessionUnderReturnedSessionId() throws
Exception {
+ ShenyuStreamableHttpServerTransportProvider provider =
providerWithRealSessions();
+
+ MockServerHttpResponse response = performRequest(provider,
+ postRequest(INITIALIZE_REQUEST_BODY, null));
+ assertEquals(HttpStatus.OK, response.getStatusCode());
+
+ final String returnedSessionId =
response.getHeaders().getFirst(SESSION_ID_HEADER);
+ assertNotNull(returnedSessionId);
+
+ final Map<String, ?> sessions = readMap(provider, "sessions");
+ final Map<String, ?> transports = readMap(provider,
"sessionTransports");
+ assertTrue(sessions.containsKey(returnedSessionId),
Review Comment:
Blocking (please act before merge): these two assertions are true on master
as well, so this test would still pass if line 333 of the provider
(`transport.setSessionId(newSessionId)`) were reverted.
Why: the registries were always keyed by `newSessionId` -
`sessions.put(newSessionId, session)` and `sessionTransports.put(newSessionId,
transport)` are unchanged context lines - and the header the client receives is
that same id (`ShenyuStreamableHttpServerTransportProvider.java:252`,
`builder.header(SESSION_ID_HEADER, result.getSessionId())`). The bug lives
entirely on the lookup side: `close()` / `closeGracefully()` call
`removeSession(this.sessionId)` with the transport's auto-UUID, and nothing
here ever calls them. So this is currently documentation, not regression
coverage.
That protection matters here because the fix depends on `sessionId` being
non-final; restoring the `final` modifier silently brings #6833 back with a
green build.
Suggested addition after these two assertions - the transport is reachable
through the map and already implements `McpServerTransport`, which declares
`close()`:
```java
final McpServerTransport transport = (McpServerTransport)
transports.get(returnedSessionId);
transport.close();
assertTrue(sessions.isEmpty(), "close() must remove the session");
assertTrue(transports.isEmpty(), "close() must remove the transport");
assertNull(ShenyuMcpExchangeHolder.get(returnedSessionId), "close() must
remove the exchange binding");
```
Please also confirm the test fails when the `setSessionId` call is removed -
that is what makes it real.
##########
shenyu-plugin/shenyu-plugin-mcp-server/src/main/java/org/apache/shenyu/plugin/mcp/server/transport/ShenyuStreamableHttpServerTransportProvider.java:
##########
@@ -1047,7 +1048,7 @@ private void completeInitializationHandshakeAsync(final
McpServerSession session
*/
private class StreamableHttpSessionTransport implements McpServerTransport
{
- private final String sessionId;
+ private String sessionId;
Review Comment:
Non-blocking: dropping `final` is the pragmatic choice given the two-phase
construction (transport first, then `session.getId()` from
`sessionFactory.create(transport)`), so I am fine with the approach. Two small
things worth considering while you are here:
- The field is read from other threads (`sendMessage` line 1115, `close` /
`closeGracefully` lines 1130/1140). Today the write at line 333 happens before
`sessionTransports.put(...)` at line 337, and every consumer reaches the
transport through that `ConcurrentHashMap`, so the happens-before edge is there
and there is no window in practice. Marking it `volatile` would make that
guarantee explicit rather than dependent on the publication order staying as it
is.
- A public setter on an otherwise construction-time field invites future
misuse (nothing guards against calling it twice, or after the session is live).
Restricting it to package-private / package-level visibility of this outer
class, or documenting "call once, before the session is published", would keep
the invariant obvious.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
