Issue #4533 Hard close from Dispatcher (#4534)

* Issue #4533 Hard close from Dispatcher

Signed-off-by: Greg Wilkins <gregw@webtide.com>

#4533 Do hard close from Dispatcher so response wrappers may intercept close.

* Issue #4533 Hard close from Dispatcher

Signed-off-by: Greg Wilkins <gregw@webtide.com>

#4533 improve test after review

* Issue #4533 Hard close from Dispatcher

Some renaming of methods to make it clear that softClose should only be used as part of sendError handling.  If softClose is used by other components, then sendError can be prevented from setting the error status.

Signed-off-by: Greg Wilkins <gregw@webtide.com>

jetty-server/src/main/java/org/eclipse/jetty/server/Dispatcher.java

@@ -219,8 +219,18 @@ public class Dispatcher implements RequestDispatcher
 
                 _contextHandler.handle(_pathInContext, baseRequest, (HttpServletRequest)request, (HttpServletResponse)response);
 
-                if (!baseRequest.getHttpChannelState().isAsync())
-                    baseRequest.getResponse().softClose();
+                // If we are not async and not closed already, then close via the possibly wrapped response.
+                if (!baseRequest.getHttpChannelState().isAsync() && !baseResponse.getHttpOutput().isClosed())
+                {
+                    try
+                    {
+                        response.getOutputStream().close();
+                    }
+                    catch (IllegalStateException e)
+                    {
+                        response.getWriter().close();
+                    }
+                }
             }
         }
         finally

jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java

@@ -905,7 +905,7 @@ public class HttpChannelState
                 throw new IllegalStateException("Response is " + _outputState);
 
             response.setStatus(code);
-            response.softClose();
+            response.errorClose();
 
             request.setAttribute(ErrorHandler.ERROR_CONTEXT, request.getErrorContext());
             request.setAttribute(ERROR_REQUEST_URI, request.getRequestURI());

jetty-server/src/main/java/org/eclipse/jetty/server/Response.java

@@ -141,12 +141,14 @@ public class Response implements HttpServletResponse
 
     public void reopen()
     {
+        // Make the response mutable and reopen output.
         setErrorSent(false);
         _out.reopen();
     }
 
-    public void softClose()
+    public void errorClose()
     {
+        // Make the response immutable and soft close the output.
         setErrorSent(true);
         _out.softClose();
     }

jetty-servlet/src/test/java/org/eclipse/jetty/servlet/DispatcherTest.java

@@ -18,7 +18,9 @@
 
 package org.eclipse.jetty.servlet;
 
+import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.OutputStream;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.EnumSet;
@@ -32,10 +34,12 @@ import javax.servlet.RequestDispatcher;
 import javax.servlet.Servlet;
 import javax.servlet.ServletContext;
 import javax.servlet.ServletException;
+import javax.servlet.ServletOutputStream;
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletRequestWrapper;
 import javax.servlet.ServletResponse;
 import javax.servlet.ServletResponseWrapper;
+import javax.servlet.WriteListener;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletRequestWrapper;
@@ -114,8 +118,9 @@ public class DispatcherTest
         String expected =
             "HTTP/1.1 200 OK\r\n" +
                 "Content-Type: text/html\r\n" +
-                "Content-Length: 0\r\n" +
-                "\r\n";
+                "Content-Length: 7\r\n" +
+                "\r\n" +
+                "FORWARD";
 
         String responses = _connector.getResponse("GET /context/ForwardServlet?do=assertforward&do=more&test=1 HTTP/1.0\n\n");
 
@@ -131,8 +136,9 @@ public class DispatcherTest
         String expected =
             "HTTP/1.1 200 OK\r\n" +
                 "Content-Type: text/html\r\n" +
-                "Content-Length: 0\r\n" +
-                "\r\n";
+                "Content-Length: 7\r\n" +
+                "\r\n" +
+                "FORWARD";
         String responses = _connector.getResponse("GET /context/ForwardServlet?do=assertforward&foreign=%d2%e5%ec%ef%e5%f0%e0%f2%f3%f0%e0&test=1 HTTP/1.0\n\n");
 
         assertEquals(expected, responses);
@@ -201,7 +207,9 @@ public class DispatcherTest
 
         String expected =
             "HTTP/1.1 200 OK\r\n" +
-                "\r\n";
+                "Content-Length: 7\r\n" +
+                "\r\n" +
+                "INCLUDE";
 
         String responses = _connector.getResponse("GET /context/IncludeServlet?do=assertinclude&do=more&test=1 HTTP/1.0\n\n");
 
@@ -217,7 +225,9 @@ public class DispatcherTest
 
         String expected =
             "HTTP/1.1 200 OK\r\n" +
-                "\r\n";
+                "Content-Length: 7\r\n" +
+                "\r\n" +
+                "INCLUDE";
 
         String responses = _connector.getResponse("GET /context/ForwardServlet/forwardpath?do=include HTTP/1.0\n\n");
 
@@ -233,7 +243,9 @@ public class DispatcherTest
 
         String expected =
             "HTTP/1.1 200 OK\r\n" +
-                "\r\n";
+                "Content-Length: 7\r\n" +
+                "\r\n" +
+                "FORWARD";
 
         String responses = _connector.getResponse("GET /context/IncludeServlet/includepath?do=forward HTTP/1.0\n\n");
 
@@ -373,6 +385,88 @@ public class DispatcherTest
         assertThat(rechoResponse, containsString("txeTohce"));
     }
 
+    @Test
+    public void testWrappedForwardCloseIntercepted() throws Exception
+    {
+        // Add filter that wraps response, intercepts close and writes after doChain
+        _contextHandler.addFilter(WrappingFilter.class, "/*", EnumSet.of(DispatcherType.REQUEST));
+        testForward();
+    }
+
+    public static class WrappingFilter implements Filter
+    {
+        @Override
+        public void init(FilterConfig filterConfig) throws ServletException
+        {
+        }
+
+        @Override
+        public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
+        {
+            ResponseWrapper wrapper = new ResponseWrapper((HttpServletResponse)response);
+            chain.doFilter(request, wrapper);
+            wrapper.sendResponse(response.getOutputStream());
+        }
+
+        @Override
+        public void destroy()
+        {
+        }
+    }
+
+    public static class ResponseWrapper extends HttpServletResponseWrapper
+    {
+        ByteArrayOutputStream buffer = new ByteArrayOutputStream();
+
+        public ResponseWrapper(HttpServletResponse response)
+        {
+            super(response);
+        }
+
+        @Override
+        public ServletOutputStream getOutputStream() throws IOException
+        {
+            return new ServletOutputStream()
+            {
+                @Override
+                public boolean isReady()
+                {
+                    return true;
+                }
+
+                @Override
+                public void setWriteListener(WriteListener writeListener)
+                {
+                    throw new UnsupportedOperationException();
+                }
+
+                @Override
+                public void write(int b) throws IOException
+                {
+                    buffer.write(b);
+                }
+
+                @Override
+                public void write(byte[] b, int off, int len) throws IOException
+                {
+                    buffer.write(b, off, len);
+                }
+
+                @Override
+                public void close() throws IOException
+                {
+                    buffer.close();
+                }
+            };
+        }
+
+        public void sendResponse(OutputStream out) throws IOException
+        {
+            out.write(buffer.toByteArray());
+            out.close();
+        }
+    }
+
     public static class ForwardServlet extends HttpServlet implements Servlet
     {
         @Override
@@ -648,6 +742,7 @@ public class DispatcherTest
 
             response.setContentType("text/html");
             response.setStatus(HttpServletResponse.SC_OK);
+            response.getOutputStream().print(request.getDispatcherType().toString());
         }
     }
 
@@ -696,6 +791,7 @@ public class DispatcherTest
 
             response.setContentType("text/html");
             response.setStatus(HttpServletResponse.SC_OK);
+            response.getOutputStream().print(request.getDispatcherType().toString());
         }
     }
 
@@ -724,6 +820,7 @@ public class DispatcherTest
 
             response.setContentType("text/html");
             response.setStatus(HttpServletResponse.SC_OK);
+            response.getOutputStream().print(request.getDispatcherType().toString());
         }
     }
 
@@ -761,6 +858,7 @@ public class DispatcherTest
 
             response.setContentType("text/html");
             response.setStatus(HttpServletResponse.SC_OK);
+            response.getOutputStream().print(request.getDispatcherType().toString());
         }
     }
 
@@ -796,6 +894,7 @@ public class DispatcherTest
 
             response.setContentType("text/html");
             response.setStatus(HttpServletResponse.SC_OK);
+            response.getOutputStream().print(request.getDispatcherType().toString());
         }
     }
 }