Salvatore Casolaro created ZOOKEEPER-5091:
---------------------------------------------
Summary: WatchManager reports connections with no active watches
after consuming the last standard watch
Key: ZOOKEEPER-5091
URL: https://issues.apache.org/jira/browse/ZOOKEEPER-5091
Project: ZooKeeper
Issue Type: Bug
Components: server
Affects Versions: 3.10.0
Environment: Operating system: Windows 11, amd64
Java version: 17.0.12
ZooKeeper version: 3.10.0-SNAPSHOT
Branch: master
Commit: 9102d0be12e66a5c592d8cbf0c6865f68ffa9252
Reporter: Salvatore Casolaro
After the last STANDARD watch associated with a connection is triggered and
consumed, WatchManager removes the active watch registration from the
path-oriented state but retains an empty entry for the connection in
watch2Paths.
As a result, getWatchesSummary() reports one connection even though it reports
zero watched paths and zero total watches. In addition, getWatches() reports
the connection session with an empty path set.
This report focuses on the externally observable inconsistency in the reporting
APIs, regardless of whether retaining an empty entry internally is intentional.
The behavior was reproduced on the Apache ZooKeeper master branch at commit
9102d0be12e66a5c592d8cbf0c6865f68ffa9252.
h2. Expected behavior
After the last STANDARD watch associated with a connection has been triggered
and consumed, the WatchManager reporting APIs should consistently reflect that
the connection has no active watch registrations.
In the reproduced scenario:
* containsWatcher(path, connection, STANDARD) should return false.
* containsWatcher(path, connection, null) should return false.
* size() should return 0.
* getWatchesSummary() should report zero active watch connections, zero
watched paths, and zero total watches.
* getWatches() should not report the connection session as having an active
watch registration.
* A second event for the same path should not notify the connection again.
This expected reporting behavior does not necessarily require removing an
internally cached empty entry from watch2Paths. If retaining the empty entry is
intentional, the reporting methods could instead exclude entries that have no
active paths.
h2. Actual behavior
The STANDARD watch is correctly triggered and consumed:
* The connection receives exactly one callback.
* containsWatcher(path, connection, STANDARD) returns false.
* containsWatcher(path, connection, null) returns false.
* size() returns 0.
* A second event does not trigger another callback.
However, getWatchesSummary() produces the following state:
{code:java}
Watch summary after trigger: connections=1, paths=0, watches=0
{code}
The regression test therefore fails with:
{code:java}
java.lang.AssertionError:
No connection with active watches must remain
Expected: 0
Actual: 1
{code}
A separate test of getWatches() also shows that the session is retained with an
empty path set:
{code:java}
java.lang.AssertionError:
A connection with no active watches must not appear in getWatches()
expected null, but was:<[]>
{code}
Therefore, the reporting APIs expose the following state:
{code:java}
containsWatcher(path, connection, STANDARD) = false
containsWatcher(path, connection, null) = false
size() = 0
getWatchesSummary().getNumConnections() = 1
getWatchesSummary().getNumPaths() = 0
getWatchesSummary().getTotalWatches() = 0
getWatches().getPaths(0x40L) = []
{code}
h2. Steps to reproduce
# Create a new WatchManager.
# Create a non-stale ServerCnxn mock with session ID 0x40.
# Register a STANDARD watch for the connection on path /a.
# Trigger a NodeDataChanged event on /a.
# Verify that the connection receives exactly one callback.
# Verify that containsWatcher() returns false and size() returns zero after
the trigger.
# Call getWatchesSummary().
# Observe that it reports one connection, zero watched paths, and zero total
watches.
# Call getWatches().
# Observe that session 0x40 is still present with an empty path set.
# Trigger another NodeDataChanged event on /a and verify that the connection
is not notified again.
h2. Minimal reproduction
The following JUnit 4 test reproduces the getWatchesSummary() behavior:
{code:java}
@Test
public void testTriggerWatch_LastStandardWatch_RemovesConnectionFromReports() {
ServerCnxn connection = createMockServerCnxn("C1", 0x40L);
List<ACL> acl = Collections.emptyList();
assertTrue(watchManager.addWatch("/a", connection, WatcherMode.STANDARD));
WatcherOrBitSet triggered = watchManager.triggerWatch("/a",
EventType.NodeDataChanged, 1L, acl, null);
assertNotNull(triggered);
assertTrue(triggered.contains(connection));
verify(connection, times(1)).process(any(WatchedEvent.class), same(acl));
assertFalse(watchManager.containsWatcher("/a", connection,
WatcherMode.STANDARD));
assertFalse(watchManager.containsWatcher("/a", connection, null));
assertEquals(0, watchManager.size());
WatchesSummary summary = watchManager.getWatchesSummary();
assertEquals("No connection with active watches must remain", 0,
summary.getNumConnections());
assertEquals(0, summary.getNumPaths());
assertEquals(0, summary.getTotalWatches());
}
{code}
The ServerCnxn mock used by the test is configured as follows:
{code:java}
private ServerCnxn createMockServerCnxn(String name, long sessionId) {
ServerCnxn connection = mock(ServerCnxn.class, name);
when(connection.getSessionId()).thenReturn(sessionId);
when(connection.isStale()).thenReturn(false);
return connection;
}
{code}
The following assertion independently reproduces the getWatches() behavior
after the same setup and trigger:
{code:java}
WatchesReport report = watchManager.getWatches();
assertNull("A connection with no active watches must not appear in
getWatches()", report.getPaths(0x40L));
{code}
The first test consistently obtains one connection instead of zero. The second
test obtains an empty set instead of null.
h2. Reproducibility
The getWatchesSummary() regression test was executed three times and produced
the same failure in each execution:
{code:java}
Expected: 0
Actual: 1
{code}
The separate getWatches() regression test was also executed three times and
produced the same result in each execution:
{code:java}
expected null, but was:<[]>
{code}
An additional diagnostic execution confirmed the complete summary state:
{code:java}
Watch summary after trigger: connections=1, paths=0, watches=0
{code}
h2. Possible cause
The behavior appears to originate in WatchManager.triggerWatch().
When the last STANDARD mode for a connection and path is consumed,
triggerWatch() removes the watcher from the path-oriented set and removes the
path from the map associated with the connection:
{code:java}
WatchStats newStats = stats.removeMode(WatcherMode.STANDARD);
if (newStats == WatchStats.NONE) {
iterator.remove();
paths.remove(localPath);
} else if (newStats != stats) {
paths.put(localPath, newStats);
}
{code}
The path entry is also removed from watchTable when its watcher set becomes
empty:
{code:java}
if (thisWatchers.isEmpty()) {
watchTable.remove(localPath);
}
{code}
However, triggerWatch() does not remove the connection key from watch2Paths
when paths becomes empty.
The resulting internal state appears to be equivalent to:
{code:java}
watchTable = {}
watch2Paths = { connection 0x40 -> {}}
{code}
getWatchesSummary() uses watch2Paths.size() as the number of connections:
{code:java}
return new WatchesSummary(watch2Paths.size(), watchTable.size(), totalWatches);
{code}
Therefore, the empty connection entry is counted even though it has no active
watch registrations.
Similarly, getWatches() iterates over all remaining watch2Paths entries and
reports the session with an empty set of paths.
Possible approaches include removing the connection from watch2Paths when its
path map becomes empty, or excluding empty entries from the reporting APIs if
retaining them internally is intentional. Any correction should preserve
connections that still have registrations on other paths or in other watch
modes.
h2. Relation to ZOOKEEPER-3131
This behavior appears closely related to, or may represent a regression or
incomplete fix of, ZOOKEEPER-3131.
ZOOKEEPER-3131 described empty watcher entries remaining in watch2Paths and
explicitly mentioned triggerWatch() as an affected cleanup path. That issue was
resolved as fixed.
The discussion also considered whether temporarily retaining an empty entry
could be useful as an optimization when clients are expected to register new
watches. Therefore, this report does not assume that removing the internal
entry is the only valid correction.
In the current implementation, however, the retained empty entry is directly
exposed through getWatchesSummary() and getWatches(), even though
containsWatcher() and size() show that no active watch registration remains.
h2. Regression tests
Two JUnit 4 regression tests reproduce the behavior using a non-stale
ServerCnxn mock:
* One verifies the state exposed by getWatchesSummary().
* One verifies the session-to-path state exposed by getWatches().
Both tests are currently marked with @Ignore in the complete experimental test
suite because they expose the reported behavior. The @Ignore annotations were
temporarily removed while reproducing the failures.
The complete regression tests can be provided if needed.
h2. Academic context
This issue was identified while working on an academic software testing project
provided within a course taught by Prof. Guglielmo De Angelis at the University
of Rome Tor Vergata.
GitHub profile:
[Prof. Guglielmo De Angelis on GitHub|https://github.com/gulyx]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
