gnodet-bot commented on code in PR #27494:
URL: https://github.com/apache/camel/pull/27494#discussion_r4208743082
##########
components/camel-ai/camel-semantic/src/test/java/org/apache/camel/semantic/SemanticInitializationTest.java:
##########
@@ -77,6 +76,38 @@ void concurrentQuestionRegistryCreationReturnsOneInstance()
throws Exception {
}
}
+ @Test
+ void cachedRegistryLookupDoesNotWaitForAnotherContext() throws Exception {
+ ExecutorService callers = Executors.newFixedThreadPool(2);
+ CountDownLatch entered = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+ try (var creating = new DefaultCamelContext(); var cached = new
DefaultCamelContext()) {
+ var expected = SemanticEvaluations.get(cached);
+
creating.getCamelContextExtension().lazyAddContextPlugin(SemanticEvaluations.class,
() -> {
+ entered.countDown();
+ try {
+ assertThat(release.await(30, TimeUnit.SECONDS)).isTrue();
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new IllegalStateException(e);
+ }
+ return null;
Review Comment:
💡 **Test does not cover simultaneous CREATION_LOCK contention; supplier
returns `null`**
The `cachedRegistryLookupDoesNotWaitForAnotherContext` test proves that a
_cached_ lookup on one context is not blocked by a concurrent _creation_ on
another (DCL fast-path works for the cached side). What is not covered: two
contexts both finding `null` on the fast-path and racing into `synchronized
(CREATION_LOCK)` simultaneously. Because `CREATION_LOCK` is `static final`,
whichever context wins the monitor serialises the other's creation globally — a
test without a pre-populated `cached` instance would confirm this path is still
a contention point.
Additionally, the lazy supplier returns `null` on line 94. Depending on
`lazyAddContextPlugin`'s contract, a `null` return may silently discard the
entry or leave it uninitialised; subsequent calls to `getContextPlugin` on the
same extension could then bypass the intended lazy initialisation path
entirely. The `assertThat(creation.get(10, TimeUnit.SECONDS)).isNotNull()`
assertion passes because `SemanticEvaluations.get()` recovers from a null
plugin by creating a fresh instance — but this recovery path is not explicitly
exercised as the test's intent.
--
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]
