Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CURATOR-729: Fix PersistentWatcher dead loop after curator closed #520

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kezhuw
Copy link
Member

@kezhuw kezhuw commented Jan 8, 2025

The dead loop is multifold:

  1. CuratorFramework::watchers does not check CuratorFrameworkState as getData or others do.
  2. PersistentWatcher loops itself through reset in BackgroundCallback.
  3. Callback in inBackground(callback).forPath(path) is invoked synchronously.

This commit enforces CuratorFrameworkState checking also to watchers, watches, sync, reconfig and getConfig.

Additionally, this commit will issue KeeperState.Closed to listeners of PersistentWatcher when curator get closed. This is not required to fix CURATOR-729, but will make the closing behavior consistent with ZooKeeper. Also, I think it is good for asynchronous Watcher.

Refs: CURATOR-529, CURATOR-673

The dead loop is multifold:
1. `CuratorFramework::watchers` does not check `CuratorFrameworkState`
   as `getData` or others do.
2. `PersistentWatcher` loops itself through `reset` in `BackgroundCallback`.
3. Callback in `inBackground(callback).forPath(path)` is invoked
   synchronously.

This commit enforces `CuratorFrameworkState` checking also to `watchers`,
`watches`, `sync`, `reconfig` and `getConfig`.

Additionally, this commit will issue `KeeperState.Closed` to listeners
of `PersistentWatcher` when curator get closed. This is not required to
fix CURATOR-729, but will make the closing behavior consistent with
ZooKeeper. Also, I think it is good for asynchronous `Watcher`.

Refs: CURATOR-529, CURATOR-673
@frinda
Copy link

frinda commented Jan 9, 2025

LGTM

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