This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 0885621ca8 Review " Clamp the backup index to the member snapshot
in..."
0885621ca8 is described below
commit 0885621ca8c869eaf2116ec1d8d576b7b293004d
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Oct 23 10:43:45 2026 +0100
Review " Clamp the backup index to the member snapshot in..."
Fix issue at source
---
.../tribes/tipis/AbstractReplicatedMap.java | 28 ++++++++++----
.../catalina/tribes/tipis/LazyReplicatedMap.java | 10 +----
.../tribes/tipis/TestLazyReplicatedMap.java | 45 ++++++++++++++--------
3 files changed, 53 insertions(+), 30 deletions(-)
diff --git a/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
b/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
index 018cadd4d5..78850f6b58 100644
--- a/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
+++ b/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
@@ -1144,11 +1144,28 @@ public abstract class AbstractReplicatedMap<K, V>
* Gets the next backup index using round-robin rotation.
*
* @return The next backup index, or -1 if no members exist
+ *
+ * @deprecated Unused. Will be removed in Tomcat 12
*/
+ @Deprecated
public int getNextBackupIndex() {
synchronized (mapMembers) {
- int size = mapMembers.size();
- if (mapMembers.isEmpty()) {
+ return getNextBackupIndex(mapMembers.size());
+ }
+ }
+
+ /**
+ * Gets the next backup index using round-robin rotation, for a list of
the given size. Use this variant with the
+ * size of a snapshot of the members so that the index is always within
that snapshot, even if members are added
+ * or removed concurrently.
+ *
+ * @param size The number of members in the list the index is used for
+ *
+ * @return The next backup index, which is less than {@code size}, or -1
if {@code size} is not positive
+ */
+ public int getNextBackupIndex(int size) {
+ synchronized (mapMembers) {
+ if (size <= 0) {
return -1;
}
int node = currentNode++;
@@ -1167,13 +1184,10 @@ public abstract class AbstractReplicatedMap<K, V>
*/
public Member getNextBackupNode() {
Member[] members = getMapMembers();
- int node = getNextBackupIndex();
- if (members.length == 0 || node == -1) {
+ int node = getNextBackupIndex(members.length);
+ if (node == -1) {
return null;
}
- if (node >= members.length) {
- node = 0;
- }
return members[node];
}
diff --git a/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
b/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
index b1b9bd207f..102eaf6317 100644
--- a/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
+++ b/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
@@ -157,19 +157,13 @@ public class LazyReplicatedMap<K, V> extends
AbstractReplicatedMap<K,V> {
return new Member[0];
}
Member[] members = getMapMembers();
- int firstIdx = getNextBackupIndex();
+ int firstIdx = getNextBackupIndex(members.length);
Member[] backup = new Member[0];
// there are no backups
- if (members.length == 0 || firstIdx == -1) {
+ if (firstIdx == -1) {
return backup;
}
- // A member may have been added between the snapshot of the members
and the computation of
- // the index, which then points beyond the end of the snapshot. Fall
back to its start, as
- // getNextBackupNode does.
- if (firstIdx >= members.length) {
- firstIdx = 0;
- }
int nextIdx = firstIdx;
boolean success = false;
diff --git a/test/org/apache/catalina/tribes/tipis/TestLazyReplicatedMap.java
b/test/org/apache/catalina/tribes/tipis/TestLazyReplicatedMap.java
index 29287c9414..facb9168ca 100644
--- a/test/org/apache/catalina/tribes/tipis/TestLazyReplicatedMap.java
+++ b/test/org/apache/catalina/tribes/tipis/TestLazyReplicatedMap.java
@@ -26,30 +26,29 @@ import org.apache.catalina.tribes.membership.MemberImpl;
public class TestLazyReplicatedMap {
/*
- * The backup node selection snapshots the members and computes an index
against the current members in two steps. A
- * member added in between lets the index point behind the end of the
snapshot, which aborted the publication with
- * an array index out of bounds exception, losing the entry that was being
stored. The index must fall back to the
- * start of the snapshot.
+ * The backup node selection snapshots the members and computes an index
in two steps. A member
+ * added in between must not let the index point behind the end of the
snapshot, which aborted
+ * the publication with an array index out of bounds exception, losing the
entry that was being
+ * stored. The index is computed for the size of the snapshot.
*/
@Test
- public void testBackupIndexBeyondMemberSnapshotFallsBack() throws
Exception {
+ public void testBackupIndexWithinMemberSnapshot() throws Exception {
Member member = new MemberImpl("localhost", 4007, -1);
+ Member other = new MemberImpl("localhost", 4008, -1);
- // A member added between the two steps of the selection, shown
directly: one member in
- // the snapshot, an index computed for a member list that has
meanwhile grown
- LazyReplicatedMap<String,String> map = new LazyReplicatedMap<>((key,
value) -> {
- // Not made primary here
- }, new GroupChannel(), 1000, "TestLazyReplicatedMap", null) {
+ LazyReplicatedMap<String,String> map = new
LazyReplicatedMap<String,String>(
+ (key, value) -> {
+ // Not made primary here
+ }, new GroupChannel(), 1000, "TestLazyReplicatedMap", null) {
@Override
public Member[] getMapMembers() {
+ // The snapshot is smaller than the member list that was just
extended
+ mapMembers.put(member, Long.valueOf(0));
+ mapMembers.put(other, Long.valueOf(0));
+ currentNode = 1;
return new Member[] { member };
}
-
- @Override
- public int getNextBackupIndex() {
- return 1;
- }
};
// The channel is not started, so the publication to the backup fails
as a channel
@@ -57,4 +56,20 @@ public class TestLazyReplicatedMap {
Member[] backup = map.publishEntryInfo("key", "value");
Assert.assertEquals(0, backup.length);
}
+
+ @Test
+ public void testNextBackupIndexClampsToSize() {
+ LazyReplicatedMap<String,String> map = new
LazyReplicatedMap<String,String>(
+ (key, value) -> {
+ // Not made primary here
+ }, new GroupChannel(), 1000, "TestLazyReplicatedMap", null);
+
+ Assert.assertEquals(-1, map.getNextBackupIndex(0));
+ Assert.assertEquals(0, map.getNextBackupIndex(3));
+ Assert.assertEquals(1, map.getNextBackupIndex(3));
+ Assert.assertEquals(2, map.getNextBackupIndex(3));
+ // Rotation index is now beyond a smaller list
+ Assert.assertEquals(0, map.getNextBackupIndex(2));
+ Assert.assertEquals(1, map.getNextBackupIndex(2));
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
