This is an automated email from the ASF dual-hosted git repository.
markt-asf pushed a commit to branch 11.0.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/11.0.x by this push:
new 824222789f Review "Skip the removal notification for keys that can't
be serialized"
824222789f is described below
commit 824222789f70c0fe5807b3daa890bc8f9c180041
Author: Mark Thomas <[email protected]>
AuthorDate: Fri Oct 23 12:06:05 2026 +0100
Review "Skip the removal notification for keys that can't be serialized"
Expand the serialization checks
Co-authored by: CoPilot / GPT-5.6 Sol
Co-authored by: Claude / Sonnet 5.5
---
.../tribes/tipis/AbstractReplicatedMap.java | 90 ++++++++++++----
.../catalina/tribes/tipis/LazyReplicatedMap.java | 16 ++-
.../catalina/tribes/tipis/LocalStrings.properties | 1 +
.../catalina/tribes/tipis/ReplicatedMap.java | 28 +++--
.../tribes/tipis/TestAbstractReplicatedMap.java | 114 +++++++++++++++++++++
5 files changed, 218 insertions(+), 31 deletions(-)
diff --git a/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
b/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
index 341331cb03..202af7d12d 100644
--- a/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
+++ b/java/org/apache/catalina/tribes/tipis/AbstractReplicatedMap.java
@@ -595,8 +595,11 @@ public abstract class AbstractReplicatedMap<K, V>
rentry.lock();
try {
// construct a diff message
- msg = new MapMessage(mapContextName,
getReplicateMessageType(), true, (Serializable) entry.getKey(),
- null, rentry.getDiff(), entry.getPrimary(),
entry.getBackupNodes());
+ msg = createMapMessage(getReplicateMessageType(), true,
(Serializable) entry.getKey(), null,
+ rentry.getDiff(), entry.getPrimary(),
entry.getBackupNodes());
+ if (msg == null) {
+ return;
+ }
rentry.resetDiff();
} catch (IOException ioe) {
log.error(sm.getString("abstractReplicatedMap.unable.diffObject"), ioe);
@@ -606,13 +609,19 @@ public abstract class AbstractReplicatedMap<K, V>
}
if (msg == null && complete) {
// construct a complete
- msg = new MapMessage(mapContextName,
getReplicateMessageType(), false, (Serializable) entry.getKey(),
+ msg = createMapMessage(getReplicateMessageType(), false,
(Serializable) entry.getKey(),
(Serializable) entry.getValue(), null,
entry.getPrimary(), entry.getBackupNodes());
+ if (msg == null) {
+ return;
+ }
}
if (msg == null) {
// construct an access message
- msg = new MapMessage(mapContextName, MapMessage.MSG_ACCESS,
false, (Serializable) entry.getKey(), null,
+ msg = createMapMessage(MapMessage.MSG_ACCESS, false,
(Serializable) entry.getKey(), null,
null, entry.getPrimary(), entry.getBackupNodes());
+ if (msg == null) {
+ return;
+ }
}
try {
if (channel != null && entry.getBackupNodes() != null &&
entry.getBackupNodes().length > 0) {
@@ -728,7 +737,12 @@ public abstract class AbstractReplicatedMap<K, V>
if (entry == null || (!entry.isSerializable())) {
return null;
}
- mapmsg.setValue((Serializable) entry.getValue());
+ try {
+ mapmsg.setValue((Serializable) entry.getValue());
+ } catch (RuntimeException x) {
+
log.error(sm.getString("abstractReplicatedMap.unable.serialize",
entry.getKey()), x);
+ return null;
+ }
return mapmsg;
}
@@ -748,10 +762,12 @@ public abstract class AbstractReplicatedMap<K, V>
// map content blocks every other state operation for a long time
otherwise
ArrayList<MapMessage> list = new ArrayList<>(snapshot.size());
for (MapState<K,V> entry : snapshot) {
- MapMessage me = new MapMessage(mapContextName, copy ?
MapMessage.MSG_COPY : MapMessage.MSG_PROXY,
- false, (Serializable) entry.key, copy ? (Serializable)
entry.value : null, null, entry.primary,
+ MapMessage me = createMapMessage(copy ? MapMessage.MSG_COPY :
MapMessage.MSG_PROXY, false,
+ (Serializable) entry.key, copy ? (Serializable)
entry.value : null, null, entry.primary,
entry.backupNodes);
- list.add(me);
+ if (me != null) {
+ list.add(me);
+ }
}
mapmsg.setValue(list);
return mapmsg;
@@ -1278,8 +1294,11 @@ public abstract class AbstractReplicatedMap<K, V>
// Skip the notification for a key that cannot be serialized, as
the message
// construction would fail on it, like the replication of put does
if (getMapMembers().length > 0 && notify && entry != null && key
instanceof Serializable) {
- MapMessage msg = new MapMessage(getMapContextName(),
MapMessage.MSG_REMOVE, false, (Serializable) key,
- null, null, null, null);
+ MapMessage msg =
+ createMapMessage(MapMessage.MSG_REMOVE, false,
(Serializable) key, null, null, null, null);
+ if (msg == null) {
+ return entry.getValue();
+ }
getChannel().send(getMapMembers(), msg,
getChannelSendOptions());
}
} catch (ChannelException x) {
@@ -1319,8 +1338,11 @@ public abstract class AbstractReplicatedMap<K, V>
backup = publishEntryInfo(key, entry.getValue());
} else if (entry.isProxy()) {
// make sure we don't retrieve from ourselves
- msg = new MapMessage(getMapContextName(),
MapMessage.MSG_RETRIEVE_BACKUP, false, (Serializable) key,
- null, null, null, null);
+ msg = createMapMessage(MapMessage.MSG_RETRIEVE_BACKUP,
false, (Serializable) key, null, null, null,
+ null);
+ if (msg == null) {
+ return null;
+ }
Response[] resp =
getRpcChannel().send(entry.getBackupNodes(), msg, RpcChannel.FIRST_REPLY,
getChannelSendOptions(), getRpcTimeout());
if (resp == null || resp.length == 0 ||
resp[0].getMessage() == null) {
@@ -1336,15 +1358,21 @@ public abstract class AbstractReplicatedMap<K, V>
}
// notify member
- msg = new MapMessage(getMapContextName(),
MapMessage.MSG_NOTIFY_MAPMEMBER, false,
- (Serializable) entry.getKey(), null, null,
channel.getLocalMember(false), backup);
+ msg = createMapMessage(MapMessage.MSG_NOTIFY_MAPMEMBER,
false, (Serializable) entry.getKey(), null,
+ null, channel.getLocalMember(false), backup);
+ if (msg == null) {
+ return null;
+ }
if (backup != null && backup.length > 0) {
getChannel().send(backup, msg,
getChannelSendOptions());
}
// invalidate the previous primary
- msg = new MapMessage(getMapContextName(),
MapMessage.MSG_PROXY, false, (Serializable) key, null,
- null, channel.getLocalMember(false), backup);
+ msg = createMapMessage(MapMessage.MSG_PROXY, false,
(Serializable) key, null, null,
+ channel.getLocalMember(false), backup);
+ if (msg == null) {
+ return null;
+ }
Member[] dest = getMapMembersExcl(backup);
if (dest != null && dest.length > 0) {
getChannel().send(dest, msg, getChannelSendOptions());
@@ -1355,8 +1383,11 @@ public abstract class AbstractReplicatedMap<K, V>
} else if (entry.isCopy()) {
backup = getMapMembers();
if (backup.length > 0) {
- msg = new MapMessage(getMapContextName(),
MapMessage.MSG_NOTIFY_MAPMEMBER, false,
- (Serializable) key, null, null,
channel.getLocalMember(false), backup);
+ msg =
createMapMessage(MapMessage.MSG_NOTIFY_MAPMEMBER, false, (Serializable) key,
null, null,
+ channel.getLocalMember(false), backup);
+ if (msg == null) {
+ return null;
+ }
getChannel().send(backup, msg,
getChannelSendOptions());
}
}
@@ -2283,6 +2314,29 @@ public abstract class AbstractReplicatedMap<K, V>
return mapContextName;
}
+ /**
+ * Creates a map message, or returns {@code null} when its key or value
cannot be serialized.
+ *
+ * @param msgtype The message type
+ * @param diff Whether this is a diff message
+ * @param key The key
+ * @param value The value
+ * @param diffvalue The serialized diff value
+ * @param primary The primary member
+ * @param nodes The backup members
+ *
+ * @return The map message, or {@code null} if serialization fails
+ */
+ protected MapMessage createMapMessage(int msgtype, boolean diff,
Serializable key, Serializable value,
+ byte[] diffvalue, Member primary, Member[] nodes) {
+ try {
+ return new MapMessage(mapContextName, msgtype, diff, key, value,
diffvalue, primary, nodes);
+ } catch (RuntimeException x) {
+ log.error(sm.getString("abstractReplicatedMap.unable.serialize",
key), x);
+ return null;
+ }
+ }
+
/**
* Gets the RPC channel.
*
diff --git a/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
b/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
index 102eaf6317..c80de55ef1 100644
--- a/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
+++ b/java/org/apache/catalina/tribes/tipis/LazyReplicatedMap.java
@@ -181,11 +181,14 @@ public class LazyReplicatedMap<K, V> extends
AbstractReplicatedMap<K,V> {
continue;
}
MapMessage msg;
+ Member[] tmpBackup = wrap(next);
+ msg = createMapMessage(MapMessage.MSG_BACKUP, false,
(Serializable) key, (Serializable) value, null,
+ channel.getLocalMember(false), tmpBackup);
+ if (msg == null) {
+ return new Member[0];
+ }
try {
- Member[] tmpBackup = wrap(next);
// publish the backup data to one node
- msg = new MapMessage(getMapContextName(),
MapMessage.MSG_BACKUP, false, (Serializable) key,
- (Serializable) value, null,
channel.getLocalMember(false), tmpBackup);
if (log.isTraceEnabled()) {
log.trace("Publishing backup data:" + msg + " to: " +
next.getName());
}
@@ -204,8 +207,11 @@ public class LazyReplicatedMap<K, V> extends
AbstractReplicatedMap<K,V> {
// publish the data out to all nodes
Member[] proxies = excludeFromSet(backup, getMapMembers());
if (proxies.length > 0) {
- msg = new MapMessage(getMapContextName(),
MapMessage.MSG_PROXY, false, (Serializable) key, null,
- null, channel.getLocalMember(false), backup);
+ msg = createMapMessage(MapMessage.MSG_PROXY, false,
(Serializable) key, null, null,
+ channel.getLocalMember(false), backup);
+ if (msg == null) {
+ return backup;
+ }
if (log.isTraceEnabled()) {
log.trace("Publishing proxy data:" + msg + " to: " +
Arrays.toNameString(proxies));
}
diff --git a/java/org/apache/catalina/tribes/tipis/LocalStrings.properties
b/java/org/apache/catalina/tribes/tipis/LocalStrings.properties
index acc249d3e0..04c6c1208d 100644
--- a/java/org/apache/catalina/tribes/tipis/LocalStrings.properties
+++ b/java/org/apache/catalina/tribes/tipis/LocalStrings.properties
@@ -41,6 +41,7 @@ abstractReplicatedMap.unable.relocate=Unable to relocate
[{0}] to a new backup n
abstractReplicatedMap.unable.remove=Unable to replicate out data for an
AbstractReplicatedMap.remove operation
abstractReplicatedMap.unable.replicate=Unable to replicate data.
abstractReplicatedMap.unable.retrieve=Unable to retrieve remote object for
key:[{0}]
+abstractReplicatedMap.unable.serialize=Unable to serialize key:[{0}] or its
value, the entry is only kept on this node
abstractReplicatedMap.unable.transferState=Unable to transfer
AbstractReplicatedMap state.
abstractReplicatedMap.unableApply.diff=Unable to apply diff to key:[{0}]
abstractReplicatedMap.unableSelect.backup=Unable to select backup node.
diff --git a/java/org/apache/catalina/tribes/tipis/ReplicatedMap.java
b/java/org/apache/catalina/tribes/tipis/ReplicatedMap.java
index 3ed7e30a4e..d72927d0e9 100644
--- a/java/org/apache/catalina/tribes/tipis/ReplicatedMap.java
+++ b/java/org/apache/catalina/tribes/tipis/ReplicatedMap.java
@@ -150,11 +150,14 @@ public class ReplicatedMap<K, V> extends
AbstractReplicatedMap<K,V> {
return backup;
}
+ MapMessage msg = createMapMessage(MapMessage.MSG_COPY, false,
(Serializable) key, (Serializable) value, null,
+ channel.getLocalMember(false), backup);
+ if (msg == null) {
+ return new Member[0];
+ }
+
try {
// publish the data out to all nodes
- MapMessage msg = new MapMessage(getMapContextName(),
MapMessage.MSG_COPY, false, (Serializable) key,
- (Serializable) value, null, channel.getLocalMember(false),
backup);
-
getChannel().send(backup, msg, getChannelSendOptions());
} catch (ChannelException e) {
FaultyMember[] faultyMembers = e.getFaultyMembers();
@@ -198,16 +201,21 @@ public class ReplicatedMap<K, V> extends
AbstractReplicatedMap<K,V> {
long start = System.currentTimeMillis();
for (Entry<K,MapEntry<K,V>> e : innerMap.entrySet()) {
MapEntry<K,V> entry = innerMap.get(e.getKey());
- if (entry == null) {
+ if (entry == null || !entry.isSerializable()) {
+ // An entry that cannot be serialized is only kept on this
node, so there is nothing to relocate
continue;
}
if (entry.isPrimary()) {
try {
Member[] backup = getMapMembers();
if (backup.length > 0) {
- MapMessage msg = new MapMessage(getMapContextName(),
MapMessage.MSG_NOTIFY_MAPMEMBER, false,
+ MapMessage msg =
createMapMessage(MapMessage.MSG_NOTIFY_MAPMEMBER, false,
(Serializable) entry.getKey(), null, null,
channel.getLocalMember(false), backup);
- getChannel().send(backup, msg,
getChannelSendOptions());
+ if (msg != null) {
+ getChannel().send(backup, msg,
getChannelSendOptions());
+ } else {
+ backup = new Member[0];
+ }
}
entry.setBackupNodes(backup);
entry.setPrimary(channel.getLocalMember(false));
@@ -228,9 +236,13 @@ public class ReplicatedMap<K, V> extends
AbstractReplicatedMap<K,V> {
entry.setCopy(false);
Member[] backup = getMapMembers();
if (backup.length > 0) {
- MapMessage msg = new MapMessage(getMapContextName(),
MapMessage.MSG_NOTIFY_MAPMEMBER, false,
+ MapMessage msg =
createMapMessage(MapMessage.MSG_NOTIFY_MAPMEMBER, false,
(Serializable) entry.getKey(), null, null,
channel.getLocalMember(false), backup);
- getChannel().send(backup, msg,
getChannelSendOptions());
+ if (msg != null) {
+ getChannel().send(backup, msg,
getChannelSendOptions());
+ } else {
+ backup = new Member[0];
+ }
}
entry.setBackupNodes(backup);
if (mapOwner != null) {
diff --git
a/test/org/apache/catalina/tribes/tipis/TestAbstractReplicatedMap.java
b/test/org/apache/catalina/tribes/tipis/TestAbstractReplicatedMap.java
index c11f38fd16..4bd792a239 100644
--- a/test/org/apache/catalina/tribes/tipis/TestAbstractReplicatedMap.java
+++ b/test/org/apache/catalina/tribes/tipis/TestAbstractReplicatedMap.java
@@ -450,4 +450,118 @@ public class TestAbstractReplicatedMap {
}
return -1;
}
+
+ /*
+ * A key may implement Serializable and still fail to serialize, which the
message construction
+ * reports as an unchecked exception. That must not abort the removal
either.
+ */
+ @Test
+ public void testRemoveUnserializableKeyDoesNotThrow() throws Exception {
+ TestChannel channel = new TestChannel();
+ LazyReplicatedMap<Object,String> map = new
LazyReplicatedMap<Object,String>(
+ (key, value) -> {
+ // Not made primary here
+ }, channel, 1000, "TestAbstractReplicatedMap", null);
+ map.memberAlive(new MemberImpl("localhost", 4019, -1));
+
+ // The put keeps the entry on this node, as it cannot be published to
a backup
+ Object key = new FailingKey();
+ map.put(key, "value");
+ Assert.assertEquals(0, channel.sendCalls.get());
+
+ Assert.assertEquals("value", map.remove(key));
+ Assert.assertNull(map.getInternal(key));
+ Assert.assertEquals(0, channel.sendCalls.get());
+ }
+
+ @Test
+ public void testPutUnserializableKeyReplicatedMapKeepsEntryLocal() throws
Exception {
+ TestChannel channel = new TestChannel();
+ ReplicatedMap<Object,String> map = new ReplicatedMap<Object,String>(
+ (key, value) -> {
+ // Not made primary here
+ }, channel, 1000, "TestAbstractReplicatedMap", null);
+ Member first = new MemberImpl("localhost", 4020, -1);
+ Member second = new MemberImpl("localhost", 4021, -1);
+ map.memberAlive(first);
+ map.memberAlive(second);
+
+ Object key = new FailingKey();
+ map.put(key, "value");
+
+ Assert.assertEquals("value", map.get(key));
+ Assert.assertEquals(0, channel.sendCalls.get());
+ map.memberDisappeared(first);
+ Assert.assertEquals("value", map.get(key));
+ Assert.assertEquals(0, channel.sendCalls.get());
+ Assert.assertEquals("value", map.remove(key));
+ }
+
+ @Test
+ public void testMemberDisappearedSkipsNonSerializableEntry() throws
Exception {
+ TestChannel channel = new TestChannel();
+ ReplicatedMap<Object,String> map = new ReplicatedMap<Object,String>(
+ (key, value) -> {
+ // Not made primary here
+ }, channel, 1000, "TestAbstractReplicatedMap", null);
+ Member first = new MemberImpl("localhost", 4024, -1);
+ Member second = new MemberImpl("localhost", 4025, -1);
+ map.memberAlive(first);
+ map.memberAlive(second);
+
+ Object key = new Object();
+ map.put(key, "value");
+ map.put("good", "value");
+
+ map.memberDisappeared(first);
+
+ Assert.assertEquals("value", map.get(key));
+ Assert.assertEquals("value", map.get("good"));
+ }
+
+ @Test
+ public void testStateRequestSkipsUnserializableEntry() throws Exception {
+ LazyReplicatedMap<Object,String> map = new
LazyReplicatedMap<Object,String>(
+ (key, value) -> {
+ // Not made primary here
+ }, new TestChannel(), 1000, "TestAbstractReplicatedMap", null);
+ map.put("good", "value");
+ map.put(new FailingKey(), "local");
+
+ MapMessage stateRequest = new
MapMessage("TestAbstractReplicatedMap".getBytes(StandardCharsets.UTF_8),
+ MapMessage.MSG_STATE_COPY, false, null, null, null, null,
null);
+ Serializable reply = map.replyRequest(stateRequest, new
MemberImpl("localhost", 4022, -1));
+
+ Assert.assertTrue(reply instanceof MapMessage);
+ Object value = ((MapMessage) reply).getValue();
+ Assert.assertTrue(value instanceof ArrayList);
+ ArrayList<?> state = (ArrayList<?>) value;
+ Assert.assertEquals(1, state.size());
+ Assert.assertEquals("good", ((MapMessage) state.get(0)).getKey());
+ }
+
+ @Test
+ public void testBackupRequestSkipsUnserializableValue() throws Exception {
+ LazyReplicatedMap<String,Object> map = new
LazyReplicatedMap<String,Object>(
+ (key, value) -> {
+ // Not made primary here
+ }, new TestChannel(), 1000, "TestAbstractReplicatedMap", null);
+ map.put("key", new FailingKey());
+
+ MapMessage backupRequest = new
MapMessage("TestAbstractReplicatedMap".getBytes(StandardCharsets.UTF_8),
+ MapMessage.MSG_RETRIEVE_BACKUP, false, "key", null, null,
null, null);
+
+ Assert.assertNull(map.replyRequest(backupRequest, new
MemberImpl("localhost", 4023, -1)));
+ }
+
+ private static final class FailingKey implements Serializable {
+
+ @Serial
+ private static final long serialVersionUID = 1L;
+
+ @Serial
+ private void writeObject(ObjectOutputStream out) throws IOException {
+ throw new IOException("Cannot be serialized");
+ }
+ }
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
