Fix #6227 Async timeout dispatch race (#6228)

Fix #6227 Async timeout dispatch race Only allow the thread calling onTimeout to call dispatch and complete once timeout has expired. Signed-off-by: Greg Wilkins <gregw@webtide.com>
jetty · May 4, 2021 · 9420863 · 9420863
1 parent 7555d03
commit 9420863
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 27 deletions.
diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java
@@ -147,6 +147,7 @@ public enum Action
     private boolean _asyncWritePossible;
     private long _timeoutMs = DEFAULT_TIMEOUT;
     private AsyncContextEvent _event;
+    private Thread _onTimeoutThread;
 
     protected HttpChannelState(HttpChannel channel)
     {
@@ -573,7 +574,10 @@ public void dispatch(ServletContext context, String path)
             switch (_requestState)
             {
                 case ASYNC:
+                    break;
                 case EXPIRING:
+                    if (Thread.currentThread() != _onTimeoutThread)
+                        throw new IllegalStateException(this.getStatusStringLocked());
                     break;
                 default:
                     throw new IllegalStateException(this.getStatusStringLocked());
@@ -637,39 +641,50 @@ protected void onTimeout()
                 throw new IllegalStateException(toStringLocked());
             event = _event;
             listeners = _asyncListeners;
+            _onTimeoutThread = Thread.currentThread();
         }
 
-        if (listeners != null)
+        try
         {
-            Runnable task = new Runnable()
+            if (listeners != null)
             {
-                @Override
-                public void run()
+                Runnable task = new Runnable()
                 {
-                    for (AsyncListener listener : listeners)
+                    @Override
+                    public void run()
                     {
-                        try
+                        for (AsyncListener listener : listeners)
                         {
-                            listener.onTimeout(event);
-                        }
-                        catch (Throwable x)
-                        {
-                            if (LOG.isDebugEnabled())
-                                LOG.warn("{} while invoking onTimeout listener {}", x.toString(), listener, x);
-                            else
-                                LOG.warn("{} while invoking onTimeout listener {}", x.toString(), listener);
+                            try
+                            {
+                                listener.onTimeout(event);
+                            }
+                            catch (Throwable x)
+                            {
+                                if (LOG.isDebugEnabled())
+                                    LOG.warn("{} while invoking onTimeout listener {}", x.toString(), listener, x);
+                                else
+                                    LOG.warn("{} while invoking onTimeout listener {}", x.toString(), listener);
+                            }
                         }
                     }
-                }
 
-                @Override
-                public String toString()
-                {
-                    return "onTimeout";
-                }
-            };
+                    @Override
+                    public String toString()
+                    {
+                        return "onTimeout";
+                    }
+                };
 
-            runInContext(event, task);
+                runInContext(event, task);
+            }
+        }
+        finally
+        {
+            synchronized (this)
+            {
+                _onTimeoutThread = null;
+            }
         }
     }
 
@@ -686,6 +701,11 @@ public void complete()
             switch (_requestState)
             {
                 case EXPIRING:
+                    if (Thread.currentThread() != _onTimeoutThread)
+                        throw new IllegalStateException(this.getStatusStringLocked());
+                    _requestState = _sendError ? RequestState.BLOCKING : RequestState.COMPLETE;
+                    break;
+
                 case ASYNC:
                     _requestState = _sendError ? RequestState.BLOCKING : RequestState.COMPLETE;
                     break;

diff --git a/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/QoSFilter.java b/jetty-servlets/src/main/java/org/eclipse/jetty/servlets/QoSFilter.java
@@ -230,8 +230,8 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
                             }
                             catch (IllegalStateException x)
                             {
-                                LOG.warn("Unable to resume suspended dispatch", x);
-                                continue;
+                                if (LOG.isDebugEnabled())
+                                    LOG.debug("dispatch failed", x);
                             }
                         }
                     }
@@ -356,12 +356,12 @@ public QoSAsyncListener(int priority)
         }
 
         @Override
-        public void onStartAsync(AsyncEvent event) throws IOException
+        public void onStartAsync(AsyncEvent event)
         {
         }
 
         @Override
-        public void onComplete(AsyncEvent event) throws IOException
+        public void onComplete(AsyncEvent event)
         {
         }
 
@@ -377,7 +377,7 @@ public void onTimeout(AsyncEvent event) throws IOException
         }
 
         @Override
-        public void onError(AsyncEvent event) throws IOException
+        public void onError(AsyncEvent event)
         {
         }
     }