IGNITE-15178 Check for clashing message group IDs in AbstractMessagingService (#242)
diff --git a/modules/network-api/src/main/java/org/apache/ignite/network/AbstractMessagingService.java b/modules/network-api/src/main/java/org/apache/ignite/network/AbstractMessagingService.java
index 33df0ed..f65762f 100644
--- a/modules/network-api/src/main/java/org/apache/ignite/network/AbstractMessagingService.java
+++ b/modules/network-api/src/main/java/org/apache/ignite/network/AbstractMessagingService.java
@@ -27,22 +27,46 @@
* Base class for {@link MessagingService} implementations.
*/
public abstract class AbstractMessagingService implements MessagingService {
+ /**
+ * Class holding a pair of a message group class and corresponding handlers.
+ */
+ private static class Handler {
+ /** */
+ final Class<?> messageGroup;
+
+ /** */
+ final List<NetworkMessageHandler> handlers;
+
+ /** */
+ Handler(Class<?> messageGroup, List<NetworkMessageHandler> handlers) {
+ this.messageGroup = messageGroup;
+ this.handlers = handlers;
+ }
+ }
+
/** Mapping from group type (array index) to a list of registered message handlers. */
- private final AtomicReferenceArray<List<NetworkMessageHandler>> handlersByGroupType =
- new AtomicReferenceArray<>(Short.MAX_VALUE + 1);
+ private final AtomicReferenceArray<Handler> handlersByGroupType = new AtomicReferenceArray<>(Short.MAX_VALUE + 1);
/** {@inheritDoc} */
@Override public void addMessageHandler(Class<?> messageGroup, NetworkMessageHandler handler) {
- handlersByGroupType.getAndUpdate(getMessageGroupType(messageGroup), handlers -> {
- if (handlers == null)
- return List.of(handler);
+ handlersByGroupType.getAndUpdate(getMessageGroupType(messageGroup), oldHandler -> {
+ if (oldHandler == null)
+ return new Handler(messageGroup, List.of(handler));
- var result = new ArrayList<NetworkMessageHandler>(handlers.size() + 1);
+ if (oldHandler.messageGroup != messageGroup) {
+ throw new IllegalArgumentException(String.format(
+ "Handlers are already registered for a message group with the same group ID " +
+ "but different class. Group ID: %d, given message group: %s, existing message group: %s",
+ getMessageGroupType(messageGroup), messageGroup, oldHandler.messageGroup
+ ));
+ }
- result.addAll(handlers);
- result.add(handler);
+ var handlers = new ArrayList<NetworkMessageHandler>(oldHandler.handlers.size() + 1);
- return result;
+ handlers.addAll(oldHandler.handlers);
+ handlers.add(handler);
+
+ return new Handler(messageGroup, handlers);
});
}
@@ -67,8 +91,8 @@
protected final Collection<NetworkMessageHandler> getMessageHandlers(short groupType) {
assert groupType >= 0 : "Group type must not be negative";
- List<NetworkMessageHandler> result = handlersByGroupType.get(groupType);
+ Handler result = handlersByGroupType.get(groupType);
- return result == null ? List.of() : result;
+ return result == null ? List.of() : result.handlers;
}
}
diff --git a/modules/network-api/src/main/java/org/apache/ignite/network/MessagingService.java b/modules/network-api/src/main/java/org/apache/ignite/network/MessagingService.java
index c524856..6ee2cfb 100644
--- a/modules/network-api/src/main/java/org/apache/ignite/network/MessagingService.java
+++ b/modules/network-api/src/main/java/org/apache/ignite/network/MessagingService.java
@@ -97,6 +97,8 @@
*
* @param messageGroup Message group descriptor.
* @param handler Message handler.
+ * @throws IllegalArgumentException If some handlers have already been registered for a different message group
+ * class that has the same ID as the given {@code messageGroup}.
*/
void addMessageHandler(Class<?> messageGroup, NetworkMessageHandler handler);
}
diff --git a/modules/network/src/test/java/org/apache/ignite/network/AbstractMessagingServiceTest.java b/modules/network/src/test/java/org/apache/ignite/network/AbstractMessagingServiceTest.java
new file mode 100644
index 0000000..2e2e166
--- /dev/null
+++ b/modules/network/src/test/java/org/apache/ignite/network/AbstractMessagingServiceTest.java
@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.network;
+
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReferenceArray;
+import org.apache.ignite.network.annotations.MessageGroup;
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.CoreMatchers.startsWith;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.Mockito.CALLS_REAL_METHODS;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.withSettings;
+
+/**
+ * Test suite for the {@link AbstractMessagingService} class.
+ */
+public class AbstractMessagingServiceTest {
+ /**
+ * Tests a situation when multiple modules declare message group descriptors with the same group ID.
+ * Adding handlers for both of these groups should result in an exception being thrown.
+ * <p>
+ * Since we can't declare multiple message groups in a single module, we have to reside to hacks using reflection.
+ */
+ @Test
+ public void testGroupIdClash() throws Exception {
+ var messagingService = mock(
+ AbstractMessagingService.class,
+ withSettings().useConstructor().defaultAnswer(CALLS_REAL_METHODS)
+ );
+
+ // get the static inner class that is stored inside the handlers list
+ Class<?> handlerClass = AbstractMessagingService.class.getDeclaredClasses()[0];
+
+ Constructor<?> constructor = handlerClass.getDeclaredConstructor(Class.class, List.class);
+ constructor.setAccessible(true);
+ // create a dummy handler
+ Object dummyHandler = constructor.newInstance(Object.class, List.of());
+
+ short groupType = TestMessageTypes.class.getAnnotation(MessageGroup.class).groupType();
+
+ // get the array of handlers and inject the dummy handler
+ Field handlersField = AbstractMessagingService.class.getDeclaredField("handlersByGroupType");
+ handlersField.setAccessible(true);
+
+ var handlers = (AtomicReferenceArray) handlersField.get(messagingService);
+ // use the groupType of the TestMessageTypes class to get a clash
+ handlers.set(groupType, dummyHandler);
+
+ Exception e = assertThrows(
+ IllegalArgumentException.class,
+ () -> messagingService.addMessageHandler(TestMessageTypes.class, (m, s, c) -> {})
+ );
+
+ assertThat(e.getMessage(), startsWith("Handlers are already registered"));
+ }
+}
