Skip to content

Adds new class for changed watcher - #6594

Open
ddanielr wants to merge 1 commit into
apache:2.1from
ddanielr:bugfix/address-naked-notify-warning
Open

ddanielr wants to merge 1 commit into
apache:2.1from
ddanielr:bugfix/address-naked-notify-warning

Conversation

@ddanielr

Copy link
Copy Markdown
Contributor

Adds a new private watcher class for looking for Changed Children. This resolves the Naked Notify warning and the var outside of scope error.

This is a change that's included in #6486 but can be broken out as a separate change.

Adds a new private watcher class for looking for Changed Children.
This resolves the Naked Notify warning and the var outside of scope
error.
@ddanielr ddanielr added this to the 2.1.7 milestone Oct 16, 2026
private final String path;
private final ReentrantLock lock = new ReentrantLock();
private final Condition childrenChanged = lock.newCondition();
private boolean changed = false;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be volatile because the ZooKeeper client thread is going to call process and the application thread is calling waitForChange.

Comment on lines +308 to +317
lock.lock();
try {
long nanos = unit.toNanos(timeout);
while (!changed && nanos > 0) {
nanos = childrenChanged.awaitNanos(nanos);
}
// Set the flag back to false so we can wait for the next change event
changed = false;
} finally {
lock.unlock();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way the locking is done here changed will never be set to true during this code block because it's waiting to acquire the same lock. I think this code will always wait the entire duration instead and then the watcher thread will acquire the lock and change the boolean value.

I think this can be simplified by using a different object type - CyclicBarrier. The code here would look something like:

    private final CyclicBarrier barrier = new CyclicBarrier(2);
      try {
        barrier.await(nanos, TimeUnit.NANOSECONDS);
        barrier.reset();
      } catch (TimeoutException e) {
      }

Then, in the process method above, it would just call barrier.await(). So, when both threads have called await on the barrier, it's true.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then, in the process method above, it would just call barrier.await(). So, when both threads have called await on the barrier, it's true.

The call to barrier.await in the process method may need to be guarded. You only want to call await if the other thread is also waiting or else it will block. You could use barrier.getNumberWaiting for this and only call await if it's greater than zero.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants