From 9bf9ad1966d8ead929d386758b5bfa20cf87fcfd Mon Sep 17 00:00:00 2001 From: themechbro Date: Thu, 26 Mar 2026 16:46:37 +0530 Subject: [PATCH 1/3] servlet: fix TomcatTransportTest detects write when not ready --- .../AsyncServletOutputStreamWriter.java | 28 +++++++++++++------ 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java b/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java index 3c8d3d07571..3f86b974467 100644 --- a/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java +++ b/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java @@ -217,7 +217,11 @@ private void assureReadyAndDrainedTurnsFalse() { */ private void runOrBuffer(ActionItem actionItem) throws IOException { WriteState curState = writeState.get(); - if (curState.readyAndDrained) { // write to the outputStream directly + + // Evaluate Tomcat's actual state alongside our cached state + boolean actualReady = curState.readyAndDrained && isReady.getAsBoolean(); + + if (actualReady) { // write to the outputStream directly actionItem.run(); if (actionItem == completeAction) { return; @@ -232,13 +236,21 @@ private void runOrBuffer(ActionItem actionItem) throws IOException { } else { // buffer to the writeChain writeChain.offer(actionItem); if (!writeState.compareAndSet(curState, curState.withReadyAndDrained(false))) { - checkState( - writeState.get().readyAndDrained, - "Bug: onWritePossible() should have changed readyAndDrained to true, but not"); - ActionItem lastItem = writeChain.poll(); - if (lastItem != null) { - checkState(lastItem == actionItem, "Bug: lastItem != actionItem"); - runOrBuffer(lastItem); + // STATE CHANGED! Determine why the CAS failed based on our initial state. + if (curState.readyAndDrained) { + // We dropped here solely because isReady() was false. + // CAS failed because another concurrent thread already CAS'd it to false. + // This is completely safe. Tomcat will call onWritePossible(). Do nothing. + } else { + // Original logic: We started as false, CAS failed because onWritePossible set it to true. + checkState( + writeState.get().readyAndDrained, + "Bug: onWritePossible() should have changed readyAndDrained to true, but not"); + ActionItem lastItem = writeChain.poll(); + if (lastItem != null) { + checkState(lastItem == actionItem, "Bug: lastItem != actionItem"); + runOrBuffer(lastItem); + } } } // state has not changed since } From b2cde71196bf07a88326304c1fa1f15f5548bf2f Mon Sep 17 00:00:00 2001 From: themechbro Date: Sat, 28 Mar 2026 22:49:14 +0530 Subject: [PATCH 2/3] chore: trigger CI pipeline --- .../AsyncServletOutputStreamWriter.java | 72 +++++++++++-------- 1 file changed, 42 insertions(+), 30 deletions(-) diff --git a/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java b/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java index 3f86b974467..85972bf9461 100644 --- a/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java +++ b/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java @@ -217,42 +217,54 @@ private void assureReadyAndDrainedTurnsFalse() { */ private void runOrBuffer(ActionItem actionItem) throws IOException { WriteState curState = writeState.get(); - - // Evaluate Tomcat's actual state alongside our cached state - boolean actualReady = curState.readyAndDrained && isReady.getAsBoolean(); - if (actualReady) { // write to the outputStream directly - actionItem.run(); - if (actionItem == completeAction) { + if (curState.readyAndDrained) { + if (isReady.getAsBoolean()) { + // Path 1: Container is truly ready. Write directly. + actionItem.run(); + if (actionItem == completeAction) { + return; + } + if (!isReady.getAsBoolean()) { + boolean successful = + writeState.compareAndSet(curState, curState.withReadyAndDrained(false)); + LockSupport.unpark(parkingThread); + checkState(successful, "Bug: curState is unexpectedly changed by another thread"); + log.finest("the servlet output stream becomes not ready"); + } return; } - if (!isReady.getAsBoolean()) { - boolean successful = - writeState.compareAndSet(curState, curState.withReadyAndDrained(false)); + } + + // Path 2: Container is secretly not ready (Tomcat bug) OR already known to be false. + // We must safely buffer the item and ensure the state reflects reality. + writeChain.offer(actionItem); + if (!writeState.compareAndSet(curState, curState.withReadyAndDrained(false))) { + // CAS failed. State changed mid-flight. + if (curState.readyAndDrained) { + // Started as true, but CAS failed because another thread + // concurrently buffered and flipped it to false. + // Safe to do nothing. The winning thread handles the unpark. + } else { + // Started as false, CAS failed because onWritePossible flipped it to true. + // Original logic: retry the write since it's ready again. + checkState( + writeState.get().readyAndDrained, + "Bug: onWritePossible() should have changed readyAndDrained to true, but not"); + ActionItem lastItem = writeChain.poll(); + if (lastItem != null) { + checkState(lastItem == actionItem, "Bug: lastItem != actionItem"); + runOrBuffer(lastItem); + } + } + } else { + // CAS succeeded! + // CRITICAL FIX: If we just flipped the state from true to false, + // we MUST wake up the container! + if (curState.readyAndDrained) { LockSupport.unpark(parkingThread); - checkState(successful, "Bug: curState is unexpectedly changed by another thread"); log.finest("the servlet output stream becomes not ready"); } - } else { // buffer to the writeChain - writeChain.offer(actionItem); - if (!writeState.compareAndSet(curState, curState.withReadyAndDrained(false))) { - // STATE CHANGED! Determine why the CAS failed based on our initial state. - if (curState.readyAndDrained) { - // We dropped here solely because isReady() was false. - // CAS failed because another concurrent thread already CAS'd it to false. - // This is completely safe. Tomcat will call onWritePossible(). Do nothing. - } else { - // Original logic: We started as false, CAS failed because onWritePossible set it to true. - checkState( - writeState.get().readyAndDrained, - "Bug: onWritePossible() should have changed readyAndDrained to true, but not"); - ActionItem lastItem = writeChain.poll(); - if (lastItem != null) { - checkState(lastItem == actionItem, "Bug: lastItem != actionItem"); - runOrBuffer(lastItem); - } - } - } // state has not changed since } } From 7f927545d87217681d1c27d3fcb7a3c446af53e0 Mon Sep 17 00:00:00 2001 From: themechbro Date: Fri, 19 Jun 2026 19:35:32 +0530 Subject: [PATCH 3/3] servlet: simplify Tomcat state mitigation to avoid complex CAS logic --- .../AsyncServletOutputStreamWriter.java | 72 ++++++++----------- 1 file changed, 28 insertions(+), 44 deletions(-) diff --git a/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java b/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java index 85972bf9461..f3f5f4da689 100644 --- a/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java +++ b/servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java @@ -35,7 +35,6 @@ import java.util.function.BooleanSupplier; import java.util.logging.Level; import java.util.logging.Logger; -import javax.annotation.Nullable; import javax.servlet.AsyncContext; import javax.servlet.ServletOutputStream; @@ -78,8 +77,6 @@ final class AsyncServletOutputStreamWriter { private final Queue writeChain = new ConcurrentLinkedQueue<>(); // for a theoretical race condition that onWritePossible() is called immediately after isReady() // returns false and before writeState.compareAndSet() - @Nullable - private volatile Thread parkingThread; AsyncServletOutputStreamWriter( AsyncContext asyncContext, @@ -202,11 +199,9 @@ private void assureReadyAndDrainedTurnsFalse() { // readyAndDrained should have been set to false already. // Just in case due to a race condition readyAndDrained is still true at this moment and is // being set to false by runOrBuffer() concurrently. - parkingThread = Thread.currentThread(); while (writeState.get().readyAndDrained) { LockSupport.parkNanos(TimeUnit.MINUTES.toNanos(1)); // should return immediately } - parkingThread = null; } /** @@ -218,52 +213,41 @@ private void assureReadyAndDrainedTurnsFalse() { private void runOrBuffer(ActionItem actionItem) throws IOException { WriteState curState = writeState.get(); + // --- NEW: Tomcat Spontaneous State Change Mitigation --- + // If our cache says true, but the container is secretly not ready, + // intercept the stale state and sync it before proceeding. + if (curState.readyAndDrained && !isReady.getAsBoolean()) { + boolean successful = writeState.compareAndSet(curState, curState.withReadyAndDrained(false)); + checkState(successful, "Bug: curState is unexpectedly changed by another thread"); + // Update local state so it gracefully bypasses the + // direct write and falls into the buffer block + curState = writeState.get(); + } + // ------------------------------------------------------- + + // The rest is the standard, original gRPC code! if (curState.readyAndDrained) { - if (isReady.getAsBoolean()) { - // Path 1: Container is truly ready. Write directly. - actionItem.run(); - if (actionItem == completeAction) { - return; - } - if (!isReady.getAsBoolean()) { - boolean successful = - writeState.compareAndSet(curState, curState.withReadyAndDrained(false)); - LockSupport.unpark(parkingThread); - checkState(successful, "Bug: curState is unexpectedly changed by another thread"); - log.finest("the servlet output stream becomes not ready"); - } + actionItem.run(); + if (actionItem == completeAction) { return; } + if (!isReady.getAsBoolean()) { + boolean successful = + writeState.compareAndSet(curState, curState.withReadyAndDrained(false)); + checkState(successful, "Bug: curState is unexpectedly changed by another thread"); + } + return; } - // Path 2: Container is secretly not ready (Tomcat bug) OR already known to be false. - // We must safely buffer the item and ensure the state reflects reality. writeChain.offer(actionItem); if (!writeState.compareAndSet(curState, curState.withReadyAndDrained(false))) { - // CAS failed. State changed mid-flight. - if (curState.readyAndDrained) { - // Started as true, but CAS failed because another thread - // concurrently buffered and flipped it to false. - // Safe to do nothing. The winning thread handles the unpark. - } else { - // Started as false, CAS failed because onWritePossible flipped it to true. - // Original logic: retry the write since it's ready again. - checkState( - writeState.get().readyAndDrained, - "Bug: onWritePossible() should have changed readyAndDrained to true, but not"); - ActionItem lastItem = writeChain.poll(); - if (lastItem != null) { - checkState(lastItem == actionItem, "Bug: lastItem != actionItem"); - runOrBuffer(lastItem); - } - } - } else { - // CAS succeeded! - // CRITICAL FIX: If we just flipped the state from true to false, - // we MUST wake up the container! - if (curState.readyAndDrained) { - LockSupport.unpark(parkingThread); - log.finest("the servlet output stream becomes not ready"); + checkState( + writeState.get().readyAndDrained, + "Bug: onWritePossible() should have changed readyAndDrained to true, but not"); + ActionItem lastItem = writeChain.poll(); + if (lastItem != null) { + checkState(lastItem == actionItem, "Bug: lastItem != actionItem"); + runOrBuffer(lastItem); } } }