sftwnd opened a new issue, #1314:
URL: https://github.com/apache/curator/issues/1314
### Summary
Since 5.9.0, `PersistentWatcher.start()` casts the client to
`org.apache.curator.framework.imps.CuratorFrameworkBase`:
```java
// This could be a namespaced facade which does not support
getCuratorListenable.
((CuratorFrameworkBase)
client).client().getCuratorListenable().addListener(...);
```
`CuratorCache` is built on `PersistentWatcher`. As a result,
`CuratorCache.start()` throws `ClassCastException` whenever the
`CuratorFramework` passed to it is not a subclass of `CuratorFrameworkBase`. A
typical case is a decorator that implements the public `CuratorFramework`
interface and delegates to a client created by `CuratorFrameworkFactory`. The
same code works with 5.8.0.
### Affected versions
- 5.9.0: the cast is in `PersistentWatcher.start()`.
- Current `master`: the same cast is in `start()` and, after #1293, also in
`close()`.
- 5.8.0 and earlier are not affected.
### Background
The cast was introduced by #1262, the fix for #1259:
- `NamespaceFacade.getCuratorListenable()` throws
`UnsupportedOperationException`.
- `PersistentWatcher` needs the `CuratorEventType.CLOSING` event
(CURATOR-729, #520).
- To get it, #1262 added `CuratorFrameworkBase.client()` and reaches the
underlying client through it.
That fixes namespaced instances, but it assumes that every
`CuratorFramework` is a `CuratorFrameworkBase`:
- `CuratorFramework` is the public interface that applications and libraries
program against. The requirement "An instance of CuratorFramework MUST BE an
instance of CuratorFrameworkBase" is stated only in the Javadoc of
`CuratorFrameworkBase`, which is in the `imps` package. The `CuratorFramework`
interface itself does not mention it.
- Before 5.9.0 no recipe relied on it, so wrappers of the public interface
worked with all recipes.
- Third-party code cannot meet the requirement. `CuratorFrameworkBase` has
package-private abstract methods (`getNamespaceImpl()`,
`getFailedDeleteManager()`, `processBackgroundOperation(...)`,
`queueOperation(...)` and others), and `DelegatingCuratorFramework` is
package-private.
### Use case
We wrap the client in a delegating `CuratorFramework`:
- It holds a client created by `CuratorFrameworkFactory` and forwards every
call to it.
- It can replace that client at runtime: it closes the old instance and
builds a new one.
- Components that received the `CuratorFramework` keep the same reference
throughout.
After the upgrade to 5.9.0, every component that uses `CuratorCache` on top
of this wrapper fails at startup. In our case this includes Spring
Integration's `ZookeeperMetadataStore` (spring-integration-zookeeper 6.5.x),
which uses `CuratorCache` internally.
Any other decorator of the interface is affected in the same way, for
example:
- metrics or tracing wrappers;
- `java.lang.reflect.Proxy`-based test doubles.
### Steps to reproduce
```java
import java.lang.reflect.InvocationTargetException;
import java.lang.reflect.Proxy;
import org.apache.curator.framework.CuratorFramework;
import org.apache.curator.framework.CuratorFrameworkFactory;
import org.apache.curator.framework.recipes.cache.CuratorCache;
import org.apache.curator.retry.RetryOneTime;
import org.apache.curator.test.TestingServer;
import org.junit.jupiter.api.Test;
public class CuratorCacheDelegatingClientTest {
@Test
public void testCuratorCacheWithDelegatingClient() throws Exception {
try (TestingServer server = new TestingServer();
CuratorFramework real =
CuratorFrameworkFactory.newClient(server.getConnectString(), new
RetryOneTime(100))) {
// A decorator that implements the public CuratorFramework
interface by delegating every call
CuratorFramework wrapper = (CuratorFramework)
Proxy.newProxyInstance(
getClass().getClassLoader(), new Class<?>[]
{CuratorFramework.class}, (proxy, method, args) -> {
try {
return method.invoke(real, args);
} catch (InvocationTargetException e) {
throw e.getCause();
}
});
wrapper.start();
try (CuratorCache cache = CuratorCache.build(wrapper, "/test")) {
cache.start(); // 5.8.0: OK, 5.9.0: ClassCastException
}
}
}
}
```
### Actual result (5.9.0)
```
java.lang.ClassCastException: class jdk.proxy2.$Proxy9 cannot be cast to
class org.apache.curator.framework.imps.CuratorFrameworkBase
(jdk.proxy2.$Proxy9 is in module jdk.proxy2 of loader 'app';
org.apache.curator.framework.imps.CuratorFrameworkBase is in unnamed module of
loader 'app')
at
org.apache.curator.framework.recipes.watch.PersistentWatcher.start(PersistentWatcher.java:85)
at
org.apache.curator.framework.recipes.cache.CuratorCacheImpl.start(CuratorCacheImpl.java:94)
at
CuratorCacheDelegatingClientTest.testCuratorCacheWithDelegatingClient(CuratorCacheDelegatingClientTest.java:27)
```
### Expected result
`CuratorCache` works with any `CuratorFramework` implementation, as it did
in 5.8.0.
If implementations that do not extend `CuratorFrameworkBase` are
intentionally unsupported, then:
- the restriction should be documented on the public `CuratorFramework`
interface;
- a violation should fail with a descriptive error instead of a
`ClassCastException`.
### Suggested fix
Use the underlying client only when the instance is a
`CuratorFrameworkBase`, which keeps the #1259 fix for namespaced facades.
Otherwise fall back to the public API, as 5.8.0 did. Against current `master`:
```diff
---
a/curator-recipes/src/main/java/org/apache/curator/framework/recipes/watch/PersistentWatcher.java
+++
b/curator-recipes/src/main/java/org/apache/curator/framework/recipes/watch/PersistentWatcher.java
@@ -87,11 +87,18 @@
public void start() {
Preconditions.checkState(state.compareAndSet(State.LATENT,
State.STARTED), "Already started");
client.getConnectionStateListenable().addListener(connectionStateListener);
- // This could be a namespaced facade which does not support
getCuratorListenable.
- ((CuratorFrameworkBase)
client).client().getCuratorListenable().addListener(clientCloseListener);
+ curatorListenable().addListener(clientCloseListener);
reset();
}
+ private Listenable<CuratorListener> curatorListenable() {
+ // A namespaced facade does not support getCuratorListenable(), so
use the underlying client (#1259).
+ // Other CuratorFramework implementations are handled through the
public API.
+ return client instanceof CuratorFrameworkBase
+ ? ((CuratorFrameworkBase)
client).client().getCuratorListenable()
+ : client.getCuratorListenable();
+ }
+
/**
* Remove the watcher
*/
@@ -99,7 +106,7 @@
public void close() {
if (state.compareAndSet(State.STARTED, State.CLOSED)) {
listeners.clear();
- ((CuratorFrameworkBase)
client).client().getCuratorListenable().removeListener(clientCloseListener);
+ curatorListenable().removeListener(clientCloseListener);
client.getConnectionStateListenable().removeListener(connectionStateListener);
try {
client.watchers().remove(watcher).guaranteed().inBackground().forPath(basePath);
```
We checked the change locally with two tests: the delegating client above,
and a namespaced client (`client.usingNamespace("ns")`, the #1259 case):
| | Delegating client | Namespaced client (#1259) |
|---|---|---|
| 5.8.0 | passes | `UnsupportedOperationException` |
| 5.9.0 | `ClassCastException` | passes |
| 5.9.0 with the same change in `start()` | passes | passes |
| `master` `PersistentWatcher` with the patch above, on 5.9.0 jars | passes
| passes |
`TestPersistentWatcher.testNamespacedWatching` already covers the namespaced
case. A test like the one above could be added next to it as a regression test
for the delegating case.
As a longer-term option, the underlying client could be exposed through the
public API, for example a method on `CuratorFramework` that facades override.
Then recipes would not need to cast to classes from the `imps` package at all.
### Environment
- Apache Curator 5.9.0 (current `master` source checked as well)
- ZooKeeper 3.9.6
- OpenJDK 25.0.4
- spring-integration-zookeeper 6.5.10
--
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]
