[GitHub] [activemq-artemis] gemmellr commented on a change in pull request #3737: ARTEMIS-3464 Improving ACK reliability on Paging and code improvements

[GitBox <gi...@apache.org>]

gemmellr commented on a change in pull request #3737:
URL: https://github.com/apache/activemq-artemis/pull/3737#discussion_r707239677



##########
File path: artemis-server/src/main/java/org/apache/activemq/artemis/core/paging/cursor/impl/PageSubscriptionImpl.java
##########
@@ -153,7 +157,7 @@ public void performScanAck() {
       // we should only have a max of 2 scheduled tasks
       // one that's might still be currently running, and another one lined up
       // no need for more than that
-      if (scheduledScanCount.incrementAndGet() < 2) {
+      if (scheduledScanCount.incrementAndGet() <= 2) {

Review comment:
       This was obviously quite broken before the change, but even afterwards it still seems brittle and could leave scans hanging around to get executed until another request comes along (assuming it does). 
   
   Its still racey because the count is only decremented by the worker thread after the the scans are actually performed, but it prepares the list of scans to operate on at the start before doing any, whilst these increment-then-decrement adjustments by the performScanAck requester threads can overlap with the whole process. This could mean that the tasks to operate on by the last executing task have already been prepared, but no task might get scheduled here by a new requester when it should have, as the number of requesters incrementing the value means that it seems there is a scheduled job that will handle it when there isnt (and they then subsequently decrement the value accordingly).
   
   I think the worker needs to update the value before it prepares its local scan list to operate on, so this counter speaks only to future scheduled scan work (it could probably be a boolean at that point) or the 'add + perform' steps collapsed into one that isnt susceptible to this issue.




-- 
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: gitbox-unsubscribe@activemq.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org