gnodet commented on code in PR #24587:
URL: https://github.com/apache/camel/pull/24587#discussion_r3559462024
##########
components/camel-ai/camel-openai/src/main/java/org/apache/camel/component/openai/OpenAIEndpoint.java:
##########
@@ -129,14 +132,13 @@ protected void doStop() throws Exception {
LOG.warn("Error closing MCP client: {}", e.getMessage(),
e);
}
}
- toolClientMap.clear();
- }
- if (cachedMcpTools != null) {
- cachedMcpTools.clear();
- }
- if (returnDirectTools != null) {
- returnDirectTools.clear();
+ toolClientMap = null;
Review Comment:
**Race condition during shutdown:** `doStop()` nulls `toolClientMap`,
`cachedMcpTools`, `returnDirectTools`, and `toolToServerName` without acquiring
the per-server locks. A concurrent `reconnectMcpServer` that has already
acquired the lock and enters `doReconnectMcpServer` will NPE on
`toolClientMap.get(toolName)` or `new HashMap<>(toolClientMap)` when the field
has been nulled.
The `BaseService` lifecycle lock and the per-server `ReentrantLock` are
independent — no ordering is enforced between `doStop` and the locked reconnect
section. This can happen during Camel context shutdown if an in-flight exchange
encounters a transport error and triggers reconnect while the context is
stopping.
Consider either:
- Acquiring all per-server locks before nulling the fields in `doStop()`
- Adding null-guards inside `doReconnectMcpServer` for each volatile field
read
- Using a `volatile boolean stopped` flag checked under the lock
--
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]
