rishabhdaim commented on code in PR #2721:
URL: https://github.com/apache/jackrabbit-oak/pull/2721#discussion_r2827566274
##########
oak-jcr/src/main/java/org/apache/jackrabbit/oak/jcr/observation/ChangeProcessor.java:
##########
@@ -460,9 +464,15 @@ public synchronized boolean stopAndWait(int timeOut,
TimeUnit unit) {
*/
public synchronized void stop() {
Validate.checkState(registration != null, "Change processor not
started");
- if (running.stop()) {
- registration.unregister();
- runningMonitor.leave();
+
+ if (stopFlagSet()) {
+ // Wait until no contentChanged is in the critical section
+ runningLock.lock();
Review Comment:
Created a test case to confirm this:
```
@Test(expected=IllegalStateException.class)
public void testStopWithoutEnterThrows() {
// Create a real ChangeProcessor instance with minimal setup using
mocks
ContentSession contentSession = Mockito.mock(ContentSession.class);
NamePathMapper namePathMapper = Mockito.mock(NamePathMapper.class);
ListenerTracker tracker = Mockito.mock(ListenerTracker.class);
FilterProvider filter = Mockito.mock(FilterProvider.class);
StatisticManager statisticManager =
Mockito.mock(StatisticManager.class);
int queueLength = 1;
CommitRateLimiter commitRateLimiter =
Mockito.mock(CommitRateLimiter.class);
BlobAccessProvider blobAccessProvider =
Mockito.mock(BlobAccessProvider.class);
ChangeProcessor changeProcessor = new ChangeProcessor(
contentSession,
namePathMapper,
tracker,
filter,
statisticManager,
queueLength,
commitRateLimiter,
blobAccessProvider
);
// Use reflection or a test hook to set registration to a dummy
CompositeRegistration
CompositeRegistration registration =
Mockito.mock(CompositeRegistration.class);
java.lang.reflect.Field regField;
try {
regField =
ChangeProcessor.class.getDeclaredField("registration");
regField.setAccessible(true);
regField.set(changeProcessor, registration);
} catch (Exception e) {
throw new RuntimeException(e);
}
// Now call stop(), which should throw due to leave() without enter()
changeProcessor.stop();
}
```
We must own the `lock` beforehand while calling `unlock`.
Now, I am really confused here, whether this code has been written
incorrectly since the beginning OR I am missing something.
--
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]
