Make strict deserialization mode fatal for IOException-wrapped failures

docs/Metrics.md

@@ -302,7 +302,7 @@ Be aware that the `__system` bolt is an actual bolt so regular bolt metrics desc
 
 `dequeuedMessages` is a throwback to older code where there was an internal queue between the server and the bolts/spouts.  That is no longer the case and the value can be ignored.
 `enqueued` is a map between the address of the remote worker and the number of tuples that were sent from it to this worker.
-`deserializationFailures` is the number of incoming messages that failed to deserialize and were dropped. When `topology.tuple.deserialization.strict.enable` is set, deserialization failures are not dropped or counted; they propagate instead. Non-IO failures terminate the worker, while failures caused by an `IOException`, such as a corrupt compressed frame, close the connection and leave the worker running.
+`deserializationFailures` is the number of incoming messages that failed to deserialize and were dropped. When `topology.tuple.deserialization.strict.enable` is set, deserialization failures are not dropped or counted; they terminate the worker instead.
 
 ##### Send (Netty Client)
 

docs/Serialization.md

@@ -131,7 +131,7 @@ Be aware that the topology-wide form enables compression for *every* remote-boun
 | `topology.tuple.compression.threshold` | `1460` | Minimum serialized tuple size, in bytes, before compression is attempted. Tuples at or below this size are sent uncompressed. The default matches the typical Ethernet TCP MSS, so payloads that already fit in a single network frame are never compressed. |
 | `storm.compression.zstd.level` | `3` | Zstd compression level. Supported range is 1–19; levels 20–22 (ultra mode) are prohibited because of their memory requirements. |
 | `topology.tuple.compression.max.decompressed.bytes` | `10485760` (10 MB) | Upper bound on the decompressed size of a single tuple. Decompression that would exceed this limit fails, guarding against malicious or corrupt payloads. |
-| `topology.tuple.deserialization.strict.enable` | `false` | Makes tuple deserialization failures fatal: an incoming message that fails to decode kills the receiving worker instead of being dropped and counted. This restores the pre-3.1.0 behavior and is mainly useful for debugging, because a persistent bad frame will put the worker in a restart loop. A failure rooted in an `IOException`, such as a corrupt or oversized compressed frame, is an exception: the receiving worker closes that connection, discards the batch it was reading, and carries on while the sender reconnects. |
+| `topology.tuple.deserialization.strict.enable` | `false` | Makes tuple deserialization failures fatal: an incoming message that fails to decode kills the receiving worker instead of being dropped and counted. This restores the pre-3.1.0 behavior and is mainly useful for debugging, because a persistent bad frame will put the worker in a restart loop. This includes failures rooted in an `IOException`, such as a corrupt or oversized compressed frame. |
 
 #### How decompression works
 

storm-client/src/jvm/org/apache/storm/Config.java

@@ -1653,10 +1653,8 @@ public class Config extends HashMap<String, Object> {
      * By default a message that fails to decode on the receiving worker is dropped and counted, and the worker keeps
      * running. When set to {@code true}, any deserialization failure propagates and the worker exits, restoring the
      * pre-3.1.0 behavior. Be aware that a single corrupt frame from a peer then kills the worker, and the supervisor
-     * restarts it into the same failure, so a persistent bad frame results in a restart loop. Failures that wrap an
-     * {@code IOException}, such as a broken or oversized compressed frame, do not reach the exit path: the messaging
-     * layer treats them like a connection error, drops the batch being read, and leaves the worker running while the
-     * peer reconnects.
+     * restarts it into the same failure, so a persistent bad frame results in a restart loop. This includes failures
+     * that wrap an {@code IOException}, such as a broken or oversized compressed frame.
      * Default: {@code false}.
      */
     @IsBoolean

storm-client/src/jvm/org/apache/storm/messaging/DeserializingConnectionCallback.java

@@ -105,10 +105,16 @@ public void recv(List<TaskMessage> batch) {
         ArrayList<AddressedTuple> ret = new ArrayList<>(batch.size());
         for (TaskMessage message : batch) {
             Tuple tuple;
+            byte[] payload = message.message();
             try {
-                tuple = des.deserialize(message.message());
+                tuple = des.deserialize(payload);
             } catch (Exception e) {
-                if (strictMode || !isToleratedDeserializationFailure(e)) {
+                if (strictMode) {
+                    int size = payload == null ? -1 : payload.length;
+                    throw new FatalDeserializationException("Failed to deserialize a message of " + size
+                                                            + " bytes destined for task " + message.task(), e);
+                }
+                if (!isToleratedDeserializationFailure(e)) {
                     throw e;
                 }
                 deserializationFailures.incrementAndGet();

storm-client/src/jvm/org/apache/storm/messaging/FatalDeserializationException.java

@@ -0,0 +1,24 @@
+/**
+ * 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.storm.messaging;
+
+/**
+ * Wraps a tuple deserialization failure under {@link org.apache.storm.Config#TOPOLOGY_TUPLE_DESERIALIZATION_STRICT_ENABLE}.
+ * The messaging layer treats it as fatal even when the cause chain contains an exception it would otherwise tolerate,
+ * such as the {@code IOException} of a corrupt compressed frame.
+ */
+public class FatalDeserializationException extends RuntimeException {
+    public FatalDeserializationException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}

storm-client/src/jvm/org/apache/storm/messaging/netty/IServer.java

@@ -18,4 +18,6 @@ public interface IServer {
     void channelActive(Channel c);
 
     void received(Object message, String remote, Channel channel) throws InterruptedException;
+
+    default void terminating() {}
 }

storm-client/src/jvm/org/apache/storm/messaging/netty/NettyUncaughtExceptionHandler.java

@@ -22,17 +22,17 @@ public class NettyUncaughtExceptionHandler implements Thread.UncaughtExceptionHa
     @Override
     public void uncaughtException(Thread t, Throwable e) {
         try {
-            LOG.error("Uncaught exception in netty " + e.getCause());
+            LOG.error("Uncaught exception in netty", e);
         } catch (Throwable err) {
             // Doing nothing (probably due to an oom issue) and hoping Utils.handleUncaughtException will handle it
         }
 
         try {
             Utils.handleUncaughtException(e);
         } catch (Throwable throwable) {
-            LOG.error("Exception thrown while handling uncaught exception " + throwable.getCause());
+            LOG.error("Exception thrown while handling uncaught exception", throwable);
         }
         LOG.info("Received error in netty thread.. terminating server...");
-        Runtime.getRuntime().exit(1);
+        Utils.exitProcessFromNewThread(1, "Uncaught exception in a netty thread");
     }
 }

storm-client/src/jvm/org/apache/storm/messaging/netty/Server.java

@@ -162,6 +162,7 @@ public int getPort() {
      */
     @Override
     public void close() {
+        closing = true;
         allChannels.close().awaitUninterruptibly();
         workerEventLoopGroup.shutdownGracefully().awaitUninterruptibly();
         bossEventLoopGroup.shutdownGracefully().awaitUninterruptibly();
@@ -270,6 +271,11 @@ public void channelActive(Channel c) {
         allChannels.add(c);
     }
 
+    @Override
+    public void terminating() {
+        closing = true;
+    }
+
     @Override
     public void received(Object message, String remote, Channel channel) throws InterruptedException {
         List<TaskMessage> msgs;

storm-client/src/jvm/org/apache/storm/messaging/netty/StormServerHandler.java

@@ -16,6 +16,7 @@
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.Set;
+import org.apache.storm.messaging.FatalDeserializationException;
 import org.apache.storm.shade.io.netty.channel.Channel;
 import org.apache.storm.shade.io.netty.channel.ChannelHandlerContext;
 import org.apache.storm.shade.io.netty.channel.ChannelInboundHandlerAdapter;
@@ -27,9 +28,16 @@ public class StormServerHandler extends ChannelInboundHandlerAdapter {
     private static final Logger LOG = LoggerFactory.getLogger(StormServerHandler.class);
     private static final Set<Class<?>> ALLOWED_EXCEPTIONS = new HashSet<>(Arrays.asList(new Class<?>[]{ IOException.class }));
     private final IServer server;
+    private final Runnable terminator;
 
     public StormServerHandler(IServer server) {
+        this(server, StormServerHandler::terminateWorker);
+    }
+
+    // Package-private for testing.
+    StormServerHandler(IServer server, Runnable terminator) {
         this.server = server;
+        this.terminator = terminator;
     }
 
     @Override
@@ -53,6 +61,12 @@ public void channelRead(ChannelHandlerContext ctx, Object msg) throws Exception
 
     @Override
     public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
+        if (Utils.exceptionCauseIsInstanceOf(FatalDeserializationException.class, cause)) {
+            // Strict deserialization mode: fatal even if the failure wraps an IOException.
+            terminate(ctx, cause, "Strict deserialization failure from {}, terminating worker");
+            ctx.close();
+            return;
+        }
         try {
             LOG.error("server errors in handling the request from {}", ctx.channel().remoteAddress().toString(), cause);
         } catch (Throwable err) {
@@ -62,9 +76,25 @@ public void exceptionCaught(ChannelHandlerContext ctx, Throwable cause) {
             Utils.handleUncaughtException(cause, ALLOWED_EXCEPTIONS, false);
             ctx.close();
         } catch (Error error) {
-            LOG.info("Received error in netty thread.. terminating server...");
-            Runtime.getRuntime().exit(1);
+            terminate(ctx, cause, "Received error in netty thread from {}, terminating server...");
+        }
+
+    }
+
+    // Stops the server delivering further batches (the event loops run until the exit completes), then exits.
+    // Logging comes last and is guarded: a failure to log (e.g. an OOM) must not skip the termination.
+    private void terminate(ChannelHandlerContext ctx, Throwable cause, String message) {
+        server.terminating();
+        terminator.run();
+        try {
+            LOG.error(message, ctx.channel().remoteAddress(), cause);
+        } catch (Throwable err) {
+            // nop
         }
+    }
 
+    // Not on this thread: the worker shutdown hook closes the netty server, which waits for this event loop.
+    private static void terminateWorker() {
+        Utils.exitProcessFromNewThread(1, "Fatal error in the netty server");
     }
 }

storm-client/src/jvm/org/apache/storm/utils/Utils.java

@@ -67,6 +67,7 @@
 import java.util.TreeMap;
 import java.util.UUID;
 import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.jar.JarFile;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
@@ -121,6 +122,7 @@
 
 public class Utils {
     public static final Logger LOG = LoggerFactory.getLogger(Utils.class);
+    private static final AtomicBoolean EXIT_REQUESTED = new AtomicBoolean();
     public static final String DEFAULT_STREAM_ID = "default";
     private static final Set<Class<?>> defaultAllowedExceptions = Collections.emptySet();
     private static final List<String> LOCALHOST_ADDRESSES = Lists.newArrayList("localhost", "127.0.0.1", "0:0:0:0:0:0:0:1");
@@ -528,6 +530,22 @@ public static void exitProcess(int val, String msg) {
         Runtime.getRuntime().exit(val);
     }
 
+    /**
+     * Exits the process like {@link #exitProcess(int, String)}, but from a new thread. Needed when the caller runs on
+     * a thread that a shutdown hook waits for, such as a Netty event loop: the worker shutdown hook closes the Netty
+     * server, which awaits its event loops, while {@code Runtime.exit()} blocks the calling thread until the hooks
+     * complete. Idempotent: once an exit is in progress, further calls return immediately instead of starting another
+     * thread that would block in {@code Runtime.exit()} forever.
+     */
+    public static void exitProcessFromNewThread(int val, String msg) {
+        if (!EXIT_REQUESTED.compareAndSet(false, true)) {
+            return;
+        }
+        Thread exitThread = new Thread(() -> exitProcess(val, msg), "process-exit");
+        exitThread.setDaemon(false);
+        exitThread.start();
+    }
+
     public static String uuid() {
         return UUID.randomUUID().toString();
     }

storm-client/test/jvm/org/apache/storm/messaging/DeserializingConnectionCallbackTest.java

@@ -170,12 +170,29 @@ public void testStrictModeMakesFailuresFatal() {
         WorkerState.ILocalTransferCallback transfer = mock(WorkerState.ILocalTransferCallback.class);
         DeserializingConnectionCallback callback = new DeserializingConnectionCallback(conf, context, transfer);
 
-        assertThrows(KryoException.class, () -> callback.recv(Collections.singletonList(taskMessage(truncated))));
+        FatalDeserializationException thrown = assertThrows(FatalDeserializationException.class,
+                                                            () -> callback.recv(Collections.singletonList(taskMessage(truncated))));
+        assertTrue(thrown.getCause() instanceof KryoException, "expected a KryoException cause but was: " + thrown.getCause());
 
         verify(transfer, never()).transfer(any());
         assertEquals(0L, callback.getAndResetDeserializationFailures());
     }
 
+    @Test
+    public void testStrictModeReportsNullPayloadAsFatal() {
+        Map<String, Object> conf = baseConf();
+        conf.put(Config.TOPOLOGY_TUPLE_DESERIALIZATION_STRICT_ENABLE, true);
+
+        WorkerState.ILocalTransferCallback transfer = mock(WorkerState.ILocalTransferCallback.class);
+        DeserializingConnectionCallback callback = new DeserializingConnectionCallback(conf, context, transfer);
+
+        FatalDeserializationException thrown = assertThrows(FatalDeserializationException.class,
+                                                            () -> callback.recv(Collections.singletonList(taskMessage(null))));
+        assertTrue(thrown.getMessage().contains("-1 bytes"), "unexpected message: " + thrown.getMessage());
+
+        verify(transfer, never()).transfer(any());
+    }
+
     @Test
     public void testStrictModeMakesUnknownTaskFailureFatal() {
         Map<String, Object> conf = baseConf();
@@ -184,8 +201,29 @@ public void testStrictModeMakesUnknownTaskFailureFatal() {
         WorkerState.ILocalTransferCallback transfer = mock(WorkerState.ILocalTransferCallback.class);
         DeserializingConnectionCallback callback = new DeserializingConnectionCallback(conf, context, transfer);
 
-        assertThrows(TupleDeserializationException.class,
-                     () -> callback.recv(Collections.singletonList(taskMessage(unknownSourceTaskTuple()))));
+        FatalDeserializationException thrown = assertThrows(FatalDeserializationException.class,
+            () -> callback.recv(Collections.singletonList(taskMessage(unknownSourceTaskTuple()))));
+        assertTrue(thrown.getCause() instanceof TupleDeserializationException,
+                   "expected a TupleDeserializationException cause but was: " + thrown.getCause());
+
+        verify(transfer, never()).transfer(any());
+        assertEquals(0L, callback.getAndResetDeserializationFailures());
+    }
+
+    @Test
+    public void testStrictModeMakesIoExceptionFailureFatal() {
+        Map<String, Object> conf = baseConf();
+        conf.put(Config.TOPOLOGY_TUPLE_DESERIALIZATION_STRICT_ENABLE, true);
+        conf.put(Config.TOPOLOGY_TUPLE_COMPRESSION_ENABLE, true);
+        byte[] fakeZstd = {(byte) 0x28, (byte) 0xB5, (byte) 0x2F, (byte) 0xFD, 0x00, 0x01, 0x02, 0x03};
+
+        WorkerState.ILocalTransferCallback transfer = mock(WorkerState.ILocalTransferCallback.class);
+        DeserializingConnectionCallback callback = new DeserializingConnectionCallback(conf, context, transfer);
+
+        FatalDeserializationException thrown = assertThrows(FatalDeserializationException.class,
+            () -> callback.recv(Collections.singletonList(taskMessage(fakeZstd))));
+        assertTrue(Utils.exceptionCauseIsInstanceOf(IOException.class, thrown),
+                   "expected an IOException in the cause chain but was: " + thrown);
 
         verify(transfer, never()).transfer(any());
         assertEquals(0L, callback.getAndResetDeserializationFailures());