jbonofre commented on code in PR #739:
URL: https://github.com/apache/camel-karaf/pull/739#discussion_r3872608128
##########
core/camel-core-osgi/src/test/java/org/apache/camel/karaf/core/OsgiTypeConverterTest.java:
##########
@@ -110,4 +125,243 @@ void removedServiceShouldInvalidateDelegate() throws
Exception {
var delegateAfter = osgiTypeConverter.getDelegate();
assertNotNull(delegateAfter);
}
+
+ @Test
+ void concurrentFirstAccessShouldBuildTheRegistryOnce() throws Exception {
+ int threads = 16;
+ AtomicInteger created = new AtomicInteger();
+ CountDownLatch startLine = new CountDownLatch(1);
+
+ OsgiTypeConverter counting = new OsgiTypeConverter(bundleContext,
camelContext, injector) {
+ @Override
+ protected DefaultTypeConverter createRegistry() {
+ created.incrementAndGet();
+ return super.createRegistry();
+ }
+ };
+
+ ExecutorService pool = Executors.newFixedThreadPool(threads);
+ try {
+ List<Future<DefaultTypeConverter>> futures = new ArrayList<>();
+ for (int i = 0; i < threads; i++) {
+ futures.add(pool.submit(() -> {
+ startLine.await();
+ return counting.getDelegate();
+ }));
+ }
+ // release them all at once so they race on the null check
+ startLine.countDown();
+
+ DefaultTypeConverter first = futures.get(0).get(30,
TimeUnit.SECONDS);
+ assertNotNull(first);
+ for (Future<DefaultTypeConverter> f : futures) {
+ assertSame(first, f.get(30, TimeUnit.SECONDS),
+ "every caller must see the same registry instance");
+ }
+ } finally {
+ pool.shutdownNow();
+ }
+
+ assertEquals(1, created.get(),
+ "the registry must be built exactly once, otherwise converters
registered on a discarded"
+ + " instance are silently lost");
+ }
+
+ /** Marker source type, so the registered converter cannot collide with a
core one. */
+ interface Marker {
+ }
+
+ @Test
+ void rebuiltRegistryReloadsTheTrackedLoadersWithoutAskingTheTracker()
throws Exception {
+ // arrives before the registry exists, so addingService only records it
+ osgiTypeConverter.addingService(serviceReference);
+ verify(loader, never()).load(any());
+
+ DefaultTypeConverter first = osgiTypeConverter.getDelegate();
+
+ // createRegistry replayed it from the recorded loaders; it never
called back into the ServiceTracker,
+ // which is what would put a framework call underneath this instance's
monitor
+ verify(loader).load(first);
+ }
+
+ @Test
+ void programmaticConverterSurvivesARegistryRebuild() throws Exception {
+ DefaultTypeConverter before = osgiTypeConverter.getDelegate();
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ assertNotNull(before.lookup(String.class, Marker.class),
"precondition: the converter is registered");
+
+ // one loader going away discards the whole registry
+ osgiTypeConverter.removedService(serviceReference, loader);
+ DefaultTypeConverter after = osgiTypeConverter.getDelegate();
+
+ assertNotSame(before, after, "the registry should have been rebuilt");
+ assertNotNull(after.lookup(String.class, Marker.class),
+ "a converter registered programmatically must be replayed onto
the rebuilt registry, otherwise it"
+ + " disappears from a running context when any bundle
unregisters a loader");
+ }
+
+ @Test
+ void removedTypeConverterIsNotResurrectedByARebuild() throws Exception {
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ osgiTypeConverter.removeTypeConverter(String.class, Marker.class);
+
+ osgiTypeConverter.removedService(serviceReference, loader);
+
+ assertNull(osgiTypeConverter.getDelegate().lookup(String.class,
Marker.class),
+ "the replay must reproduce the sequence, not just the
additions");
+ }
+
+ @Test
+ void addingServiceReleasesTheServiceWhenLoadingFails() throws Exception {
+ osgiTypeConverter.getDelegate();
+ doThrow(new RuntimeException("boom")).when(loader).load(any());
+
+ assertThrows(RuntimeCamelException.class, () ->
osgiTypeConverter.addingService(serviceReference));
+
+ // a customizer that throws is treated as never tracked, so
removedService will not run for this
+ // reference and nothing else would release the use count taken by
addingService
+ verify(bundleContext).ungetService(serviceReference);
+ }
+
+ @Test
+ void removedServiceReleasesTheService() {
+ osgiTypeConverter.addingService(serviceReference);
+
+ osgiTypeConverter.removedService(serviceReference, loader);
+
+ verify(bundleContext).ungetService(serviceReference);
+ }
+
+ @Test
+ void addingServiceMustNotHoldTheInstanceMonitor() throws Exception {
+ // build first, so addingService takes the branch that calls into the
loader
+ osgiTypeConverter.getDelegate();
+
+ CountDownLatch insideLoad = new CountDownLatch(1);
+ CountDownLatch releaseLoad = new CountDownLatch(1);
+ doAnswer(invocation -> {
+ insideLoad.countDown();
+ releaseLoad.await(30, TimeUnit.SECONDS);
+ return null;
+ }).when(loader).load(any());
+
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+ try {
+ Future<?> adding = pool.submit(() ->
osgiTypeConverter.addingService(serviceReference));
+ assertTrue(insideLoad.await(30, TimeUnit.SECONDS), "addingService
should have reached loader.load");
+
+ // the framework calls addingService while it is dispatching a
service event; if it took this
+ // instance's monitor, every conversion in the container would
block behind an arbitrary bundle's
+ // loader for as long as that loader takes
+ Future<DefaultTypeConverter> reader =
pool.submit(osgiTypeConverter::getDelegate);
+ assertNotNull(reader.get(10, TimeUnit.SECONDS),
+ "getDelegate must not be blocked by an in-flight
addingService");
+
+ releaseLoad.countDown();
+ adding.get(30, TimeUnit.SECONDS);
+ } finally {
+ releaseLoad.countDown();
+ pool.shutdownNow();
+ }
+ }
+
+ @Test
+ void addRemovePairsDoNotAccumulate() {
+ osgiTypeConverter.getDelegate();
+
+ for (int i = 0; i < 500; i++) {
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ osgiTypeConverter.removeTypeConverter(String.class, Marker.class);
+ }
+
+ // an inverse-appending list would sit at 1000 here, and every
converter it captured - and the classloader
+ // of the bundle that contributed it - would stay strongly reachable
for the life of the context
+ assertEquals(0, osgiTypeConverter.programmaticRegistrationCount(),
+ "add/remove pairs must prune, not accumulate");
+ }
+
+ @Test
+ void reRegisteringTheSameConversionReplaces() {
+ osgiTypeConverter.getDelegate();
+
+ for (int i = 0; i < 10; i++) {
+ // what a Blueprint container refresh looks like: the same
conversion registered again
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ }
+
+ assertEquals(1, osgiTypeConverter.programmaticRegistrationCount(),
+ "re-registering the same conversion must replace rather than
append");
+ }
+
+ @Test
+ void removingAConversionThatWasNeverRegisteredDoesNotAccumulate() {
+ osgiTypeConverter.getDelegate();
+
+ osgiTypeConverter.removeTypeConverter(String.class, Marker.class);
+
+ assertEquals(0, osgiTypeConverter.programmaticRegistrationCount(),
+ "a removal for a pair that was never registered must not be
retained");
+ }
+
+ @Test
+ void restartDoesNotReplayThePreviousLifecycle() throws Exception {
+ osgiTypeConverter.start();
+ osgiTypeConverter.addTypeConverter(String.class, Marker.class,
typeConverter);
+ assertNotNull(osgiTypeConverter.getDelegate().lookup(String.class,
Marker.class));
+
+ osgiTypeConverter.stop();
+ osgiTypeConverter.start();
+
+ // the converters captured before the stop belong to bundles that may
be gone by now
+ assertEquals(0, osgiTypeConverter.programmaticRegistrationCount());
+ assertNull(osgiTypeConverter.getDelegate().lookup(String.class,
Marker.class),
+ "a stop/start cycle must not resurrect the previous
lifecycle's registrations");
+ }
+
+ @Test
+ void registrationCannotInterleaveWithARebuild() throws Exception {
+ CountDownLatch insideBuild = new CountDownLatch(1);
+ CountDownLatch releaseBuild = new CountDownLatch(1);
+
+ OsgiTypeConverter stalling = new OsgiTypeConverter(bundleContext,
camelContext, injector) {
+ @Override
+ protected DefaultTypeConverter createRegistry() {
+ DefaultTypeConverter built = super.createRegistry();
+ insideBuild.countDown();
+ try {
+ releaseBuild.await(30, TimeUnit.SECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ }
+ return built;
+ }
+ };
+
+ ExecutorService pool = Executors.newFixedThreadPool(2);
+ try {
+ Future<DefaultTypeConverter> builder =
pool.submit(stalling::getDelegate);
+ assertTrue(insideBuild.await(30, TimeUnit.SECONDS), "the rebuild
should have started");
+
+ Future<?> registrar = pool.submit(() -> {
+ stalling.addTypeConverter(String.class, Marker.class,
typeConverter);
+ return null;
+ });
+
+ // apply-and-record has to be one step against the rebuild. If it
were not, this registration would be
+ // applied to the registry being discarded and only recorded
afterwards, so the rebuilt one would
+ // neither have it applied nor replay it
+ assertThrows(TimeoutException.class, () -> registrar.get(2,
TimeUnit.SECONDS),
Review Comment:
**Proving this by waiting 2 s for a `TimeoutException` is a `Thread.sleep`
in disguise, and it passes vacuously if the second pool thread has not yet
entered `addTypeConverter`.**
Two problems:
1. It adds a hard 2 s to every build and asserts a negative by elapsed time
— the pattern the project's Awaitility guidance exists to eliminate ("flaky,
slow, and non-deterministic").
2. It is not actually a guard. On a loaded CI box or with a cold thread
pool, the `registrar` thread may not have reached the synchronized `register()`
at all; the `get()` times out anyway and the test reports success even against
an implementation where `register()` is **not** synchronized. The PR
description already notes this test passes against its predecessor.
A deterministic positive assertion is available and the test almost makes it
already: release the build, join both futures, then assert the registration is
present in the published registry — which lines 358-359 do. Dropping the timing
assertion loses nothing and removes the 2 s.
_AI-generated review on behalf of JB Onofré_
--
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]
