Fixes #6693 - FastCGI review

Updates after review. Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
jetty · Sep 3, 2021 · 3580236 · 3580236
1 parent c36746e
commit 3580236
Show file tree

Hide file tree

Showing 3 changed files with 31 additions and 19 deletions.
diff --git a/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/Parser.java b/jetty-fcgi/fcgi-client/src/main/java/org/eclipse/jetty/fcgi/parser/Parser.java
@@ -106,11 +106,8 @@ public boolean parse(ByteBuffer buffer)
                             // parsing; the async operation will eventually resume parsing.
                             return true;
                         }
-                        else
-                        {
-                            padding = headerParser.getPaddingLength();
-                            state = State.PADDING;
-                        }
+                        padding = headerParser.getPaddingLength();
+                        state = State.PADDING;
                     }
                     break;
                 }

diff --git a/jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java b/jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/HttpChannelOverFCGI.java
@@ -41,6 +41,7 @@
 public class HttpChannelOverFCGI extends HttpChannel
 {
     private static final Logger LOG = LoggerFactory.getLogger(HttpChannelOverFCGI.class);
+    private static final HttpInput.Content EOF_CONTENT = new HttpInput.EofContent();
 
     private final Queue<HttpInput.Content> _contentQueue = new LinkedList<>();
     private final Callback _asyncFillCallback = new AsyncFillCallback();
@@ -69,7 +70,7 @@ public boolean onContent(HttpInput.Content content)
         Throwable failure = _specialContent == null ? null : _specialContent.getError();
         if (failure == null)
             _contentQueue.offer(content);
-        if (failure != null)
+        else
             content.failed(failure);
 
         return result;
@@ -78,31 +79,40 @@ public boolean onContent(HttpInput.Content content)
     @Override
     public boolean needContent()
     {
-        if (needContent("immediate"))
+        if (hasContent())
+        {
+            if (LOG.isDebugEnabled())
+                LOG.debug("needContent has immediate content {}", this);
             return true;
+        }
+
         parseAndFill();
-        if (needContent("parsed"))
-            return true;
-        connection.getEndPoint().tryFillInterested(_asyncFillCallback);
-        return false;
-    }
 
-    private boolean needContent(String context)
-    {
-        if (_specialContent != null || !_contentQueue.isEmpty())
+        if (hasContent())
         {
             if (LOG.isDebugEnabled())
-                LOG.debug("needContent has {} content {}", context, this);
+                LOG.debug("needContent has parsed content {}", this);
             return true;
         }
+
+        connection.getEndPoint().tryFillInterested(_asyncFillCallback);
         return false;
     }
 
+    private boolean hasContent()
+    {
+        return _specialContent != null || !_contentQueue.isEmpty();
+    }
+
     @Override
     public HttpInput.Content produceContent()
     {
-        if (_specialContent == null && _contentQueue.isEmpty())
+        if (!hasContent())
             parseAndFill();
+
+        if (!hasContent())
+            return null;
+
         HttpInput.Content content = _contentQueue.poll();
         if (content != null)
         {
@@ -167,7 +177,7 @@ protected boolean eof()
     {
         if (LOG.isDebugEnabled())
             LOG.debug("received EOF");
-        _specialContent = new HttpInput.EofContent();
+        _specialContent = EOF_CONTENT;
         return getRequest().getHttpInput().onContentProducible();
     }
 
@@ -263,10 +273,10 @@ private boolean doOnIdleTimeout(Throwable x)
     @Override
     public void recycle()
     {
+        super.recycle();
         if (!_contentQueue.isEmpty())
             throw new AssertionError("unconsumed content: " + _contentQueue);
         _specialContent = null;
-        super.recycle();
     }
 
     @Override

diff --git a/jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/ServerFCGIConnection.java b/jetty-fcgi/fcgi-server/src/main/java/org/eclipse/jetty/fcgi/server/ServerFCGIConnection.java
@@ -133,10 +133,13 @@ void parseAndFill()
     {
         if (LOG.isDebugEnabled())
             LOG.debug("parseAndFill {}", this);
+        // This loop must run only until the request is completed.
+        // See also HttpConnection.parseAndFillForContent().
         while (channel != null)
         {
             if (parse(networkBuffer.getBuffer()))
                 return;
+            // Check if the request was completed by the parsing.
             if (channel == null)
                 return;
             if (fillInputBuffer() <= 0)
@@ -265,6 +268,8 @@ public void onEnd(int request)
             {
                 channel.onContentComplete();
                 channel.onRequestComplete();
+                // Nulling out the channel signals that the
+                // request is complete, see also parseAndFill().
                 channel = null;
             }
         }