Copilot commented on code in PR #53:
URL:
https://github.com/apache/sling-org-apache-sling-event/pull/53#discussion_r2879267011
##########
src/main/java/org/apache/sling/event/impl/jobs/JobConsumerManager.java:
##########
@@ -370,6 +370,9 @@ private void unbindService(final ServiceReference<?>
serviceReference, final boo
if (topics != null && topics.length > 0) {
final ConsumerInfo info = new ConsumerInfo(serviceReference,
isConsumer);
boolean changed = false;
+ // Collect async callbacks to invoke outside the lock to avoid
+ // lock-order inversion between topicToConsumerMap and
QueueJobCache.cache
+ final List<JobExecutionContext> asyncCallbacks = new ArrayList<>();
Review Comment:
This deadlock fix changes callback invocation timing in `unbindService()`,
but current `JobConsumerManagerTest` coverage doesn’t exercise the
listener/callback path. Consider adding a unit test that registers a listener +
`JobExecutionContext` and verifies unbind triggers `asyncProcessingFinished`
(and can re-enter `JobConsumerManager` without deadlocking, using a
latch/timeout).
##########
src/main/java/org/apache/sling/event/impl/jobs/JobConsumerManager.java:
##########
@@ -404,6 +404,10 @@ private void unbindService(final ServiceReference<?>
serviceReference, final boo
this.calculateTopics(this.propagationService != null);
}
}
+ // notify listeners outside the lock
+ for (final JobExecutionContext context : asyncCallbacks) {
+ context.asyncProcessingFinished(context.result().failed());
Review Comment:
`JobExecutionContextImpl.asyncProcessingFinished(...)` throws
`IllegalStateException` when the job is not async or has already been finished.
Since callbacks are now invoked after releasing `topicToConsumerMap`, there is
a larger race window where the job can finish before this loop runs, causing
`unbindService()` to throw and potentially disrupt DS unbind / property
updates. Consider catching `IllegalStateException` per callback (and logging at
debug) so unbind remains best-effort.
```suggestion
try {
context.asyncProcessingFinished(context.result().failed());
} catch (final IllegalStateException ise) {
logger.debug(
"Ignoring IllegalStateException while finishing
async processing during unbind of {}",
serviceReference, ise);
}
```
--
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]
