[CURATOR-495] Race and possible dead locks with RetryPolicies and several Curator Recipes

[jira-importer]

In trying to figure out why TestInterProcessSemaphoreMutex is so flakey I've come across a fairly serious edge case in how several of our recipes work. You can see the issue in InterProcessSemaphoreV2 (which is what InterProcessSemaphoreMutex uses internally). Look here:

InterProcessSemaphoreV2.java

The code synchronizes and then does client.getChildren().... This is where the problem is. If there are connection problems inside of getChildren() the retry policy will do configured sleeping, retries, etc. Importantly, this is all done while the thread doing the retries holds InterProcessSemaphoreV2's monitor. If the ZK connection is repaired past the session timeout, ZK will eventually call InterProcessSemaphoreV2's watcher with an Expired message. InterProcessSemaphoreV2's watcher calls this method:

private synchronized void notifyFromWatcher()
{
    notifyAll();
}

You can see that this is a race. The thread doing "getChildren" is holding the monitor and is in a retry loop waiting for the connection to be repaired. However, ZK's event loop is trying to obtain that same monitor as a result of trying to call the synchronized notifyFromWatcher(). This means that the retry policy will always fail because ZK's event loop is tied up until that thread exists. Worse still, if someone were to use a retry policy of "RetryForever" they'd have a deadlock.

This pattern is in about 10 files or so. I'm trying to think of a workaround. One possibility is to use a separate thread for this type of notification. i.e. notifyFromWatcher() would just signal another thread that the notifyAll() needs to be called. This would unblock ZK's event thread so that things can progress. I'll play around with this.

---

Originally reported by randgalt, imported from: Race and possible dead locks with RetryPolicies and several Curator Recipes

• assignee: randgalt
• status: Resolved
• priority: Blocker
• resolution: Fixed
• resolved: 2018-12-17T03:26:55+00:00
• imported: 2025-01-21

[jira-importer]

githubbot:

GitHub user Randgalt opened a pull request:

#297

CURATOR-495 Fixes race in many Curator recipes which could block ZK's event thread

Fixes race in many Curator recipes whereby a pattern was used that called "notifyAll" in a synchronized block in response to a ZooKeeper watcher callback. This created a race and possible deadlock if the recipe instance was already in a synchronized block. This would result in the ZK event thread getting blocked which would prevent ZK connections from getting repaired. This change adds a new executor (available from CuratorFramework) that can be used to do the sync/notify so that ZK's event thread is not blocked.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/apache/curator CURATOR-495

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/curator/pull/297.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #297

---

commit f91adb2
Author: randgalt
Date: 2018-12-14T01:34:40Z

CURATOR-495

Fixes race in many Curator recipes whereby a pattern was used that called "notifyAll" in a synchronized block in response to a ZooKeeper watcher callback. This created a race and possible deadlock if the recipe instance was already in a synchronized block. This would result in the ZK event thread getting blocked which would prevent ZK connections from getting repaired. This change adds a new executor (available from CuratorFramework) that can be used to do the sync/notify so that ZK's event thread is not blocked.

---

[jira-importer]

githubbot:

Github user eolivelli commented on a diff in the pull request:

#297 (comment)

— Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFramework.java —
@@ -331,4 +329,29 @@

• @return true/false
  */
  boolean isZk34CompatibilityMode();
  +
  + /**
  + * Calls {@link #notifyAll()}

  on the given object after first synchronizing on it. This is
  + * done from the

  {@link #runSafe(Runnable)}

  thread.
  + *
  + * @param monitorHolder object to sync on and notify
  + * @SInCE 4.1.0
  + */
  + default void postSafeNotify(Object monitorHolder)

  • • End diff –

Thinking about the future...
Would it be better to make it return a CompletableFuture?
This way the caller will have a chance to know when eventually the operation has been executed

[jira-importer]

githubbot:

Github user Randgalt commented on a diff in the pull request:

#297 (comment)

— Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFramework.java —
@@ -331,4 +329,29 @@

• @return true/false
  */
  boolean isZk34CompatibilityMode();
  +
  + /**
  + * Calls {@link #notifyAll()}

  on the given object after first synchronizing on it. This is
  + * done from the

  {@link #runSafe(Runnable)}

  thread.
  + *
  + * @param monitorHolder object to sync on and notify
  + * @SInCE 4.1.0
  + */
  + default void postSafeNotify(Object monitorHolder)

  • • End diff –

Good idea - I'll add that

[jira-importer]

githubbot:

Github user cammckenzie commented on a diff in the pull request:

#297 (comment)

— Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java —
@@ -474,6 +477,28 @@ public Builder schemaSet(SchemaSet schemaSet)
return this;
}

+/**
+ * Curator (and user) recipes will use this executor to call notifyAll
+ * and other blocking calls that might normally block ZooKeeper's event thread.
+ * By default, an executor is allocated internally using the provided (or default)
+ *

{@link #threadFactory(java.util.concurrent.ThreadFactory)}

. Use this method
+ * to set a custom executor.
+ *
+ * @param runSafeService executor to use for calls to notifyAll from Watcher callbacks etc
+ * @return this
+ * @SInCE 4.1.0
+ */
+public Builder runSafeService(Executor runSafeService)
+{
+ this.runSafeService = runSafeService;
+ return null;
— End diff –

This should return this rather than null

[jira-importer]

githubbot:

Github user Randgalt commented on a diff in the pull request:

#297 (comment)

— Diff: curator-framework/src/main/java/org/apache/curator/framework/CuratorFrameworkFactory.java —
@@ -474,6 +477,28 @@ public Builder schemaSet(SchemaSet schemaSet)
return this;
}

+/**
+ * Curator (and user) recipes will use this executor to call notifyAll
+ * and other blocking calls that might normally block ZooKeeper's event thread.
+ * By default, an executor is allocated internally using the provided (or default)
+ *

{@link #threadFactory(java.util.concurrent.ThreadFactory)}

. Use this method
+ * to set a custom executor.
+ *
+ * @param runSafeService executor to use for calls to notifyAll from Watcher callbacks etc
+ * @return this
+ * @SInCE 4.1.0
+ */
+public Builder runSafeService(Executor runSafeService)
+{
+ this.runSafeService = runSafeService;
+ return null;
— End diff –

damn - good catch

[jira-importer]

githubbot:

Github user cammckenzie commented on the issue:

#297

I have just run the tests and everything passed, which is a bit of a novelty! Still failed at the end with that surefire issue. I think that this is good to merge though. Nice work!

[jira-importer]

githubbot:

Github user Randgalt commented on the issue:

#297

For the record: here’s the Surefire big report: https://issues.apache.org/jira/browse/SUREFIRE-1612

[jira-importer]

githubbot:

Github user asfgit closed the pull request at:

#297

[jira-importer]

githubbot:

Github user eolivelli commented on the issue:

#297

Did you try to upgrade testng and surefire to latest and greatest version?

[jira-importer]

githubbot:

Github user Randgalt commented on the issue:

#297

> Did you try to upgrade testng and surefire to latest and greatest version

Surefire was updated as part of getting the latest Apache POM. I'll play around with latest TestNG

[jira-importer]

githubbot:

Github user Randgalt commented on the issue:

#297

FYI - I just tried it with the latest TestNg and it still fails. So, I re-opened my issue there: testng-team/testng#1976