paulrutter commented on code in PR #576:
URL: https://github.com/apache/felix-dev/pull/576#discussion_r4281975256
##########
framework/src/main/java/org/apache/felix/framework/BundleWiringImpl.java:
##########
@@ -2156,6 +2156,12 @@ Class<?> defineClassParallel(String name, Felix felix,
Set<ServiceReference<Wove
m_classLocks.remove(name);
Review Comment:
Nit: this `finally` still uses the unconditional
`m_classLocks.remove(name)`, while the new branch uses the owner-checked
`remove(name, me)`. Using `remove(name, me)` in both places keeps release
owner-safe. If the loop's exit conditions ever change so the try block runs
without this thread owning the lock, the unconditional remove would drop
another thread's entry.
##########
framework/src/test/java/org/apache/felix/framework/BundleWiringImplTest.java:
##########
@@ -163,6 +163,48 @@ void findClassExistant() throws Exception
assertThat(foundClass).as("Class Should be found in this
classloader").isNotNull();
}
+ @Test
+ void defineClassParallelReleasesItsLockWhenTheClassIsAlreadyDefined()
throws Exception
+ {
+ Felix mockFramework = mock(Felix.class);
Review Comment:
Nit: this setup (about 20 lines of Felix/HookRegistry/Content mocking, bytes
creation and classloader creation) is copied from `findClassExistant` and
similar tests. A small shared helper would help here.
##########
framework/src/test/java/org/apache/felix/framework/BundleWiringImplTest.java:
##########
@@ -163,6 +163,48 @@ void findClassExistant() throws Exception
assertThat(foundClass).as("Class Should be found in this
classloader").isNotNull();
}
+ @Test
+ void defineClassParallelReleasesItsLockWhenTheClassIsAlreadyDefined()
throws Exception
+ {
+ Felix mockFramework = mock(Felix.class);
+ HookRegistry hReg = mock(HookRegistry.class);
+ Mockito.when(mockFramework.getHookRegistry()).thenReturn(hReg);
+ Content mockContent = mock(Content.class);
+ Class<?> testClass = TestClass.class;
+ String testClassName = testClass.getName();
+ String testClassAsPath = testClassName.replace('.', '/') + ".class";
+ byte[] testClassBytes = createTestClassBytes(testClass,
testClassAsPath);
+
+ List<Content> contentPath = new ArrayList<>();
+ contentPath.add(mockContent);
+ initializeSimpleBundleWiring();
+
+ when(mockBundle.getFramework()).thenReturn(mockFramework);
+ when(mockFramework.getBootPackages()).thenReturn(new String[0]);
+
+ when(mockRevisionImpl.getContentPath()).thenReturn(contentPath);
+ when(mockContent.getEntryAsBytes(testClassAsPath)).thenReturn(
+ testClassBytes);
+
+ BundleClassLoader bundleClassLoader = createBundleClassLoader(
+ BundleClassLoader.class, bundleWiring);
+ Class<?> defined = bundleClassLoader.findClass(testClassName);
+ assertThat(defined).as("Class should be defined by this
classloader").isNotNull();
+
+ // A thread that read the class bytes before another thread defined
the class arrives
+ // here once the class exists: it takes the free lock, finds the class
and returns it.
+ Class<?> found = bundleClassLoader.defineClassParallel(testClassName,
mockFramework,
+ Collections.emptySet(), null, testClassBytes, mockContent,
+ testClassName.substring(0, testClassName.lastIndexOf('.')));
+
+ assertThat(found).as("The class already defined should be
returned").isSameAs(defined);
+ Field classLocks =
BundleClassLoader.class.getDeclaredField("m_classLocks");
+ classLocks.setAccessible(true);
+ assertThat((Map<?, ?>) classLocks.get(bundleClassLoader))
+ .as("No class lock should outlive the define, or it keeps its
thread reachable")
+ .isEmpty();
Review Comment:
This covers the "we took the free lock and found the class" path well. The
contended path (another thread owns the lock, class found while it spins) is
untested. If someone later turns `remove(name, me)` into `remove(name)`,
nothing would fail, and another thread's lock could be dropped mid-define. A
test that pre-seeds `m_classLocks` with a different thread and asserts the
entry survives would guard that.
##########
framework/src/main/java/org/apache/felix/framework/BundleWiringImpl.java:
##########
@@ -2156,6 +2156,12 @@ Class<?> defineClassParallel(String name, Felix felix,
Set<ServiceReference<Wove
m_classLocks.remove(name);
}
}
+ else
+ {
+ // This thread may have taken the lock (putIfAbsent returned
null) and then found
+ // the class already defined by another thread, so release it
if it is ours.
+ m_classLocks.remove(name, me);
Review Comment:
This is correct. A more general shape would avoid the compensating `else`.
Acquisition is a side effect of the `while` condition, so release has to be
re-added on every exit path. Keeping the lock release in a single `finally`
that always runs `m_classLocks.remove(name, me)` would cover this leak and any
future exit path. Fine to keep the minimal fix if you prefer.
--
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]
