diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 11b0f74a2fb..cc5dbb023b8 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -373,7 +373,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor if (!_request.hasMetaData()) throw new IllegalStateException("state=" + _state); _request.setHandled(false); - _response.getHttpOutput().reopen(); + _response.reopen(); dispatch(DispatcherType.REQUEST, () -> { @@ -392,7 +392,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor case ASYNC_DISPATCH: { _request.setHandled(false); - _response.getHttpOutput().reopen(); + _response.reopen(); dispatch(DispatcherType.ASYNC,() -> getServer().handleAsync(this)); break; @@ -409,7 +409,7 @@ public class HttpChannel implements Runnable, HttpOutput.Interceptor // Get ready to send an error response _request.setHandled(false); _response.resetContent(); - _response.getHttpOutput().reopen(); + _response.reopen(); // the following is needed as you cannot trust the response code and reason // as those could have been modified after calling sendError 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 index d75eb7a7e5b..53e302d6311 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannelState.java @@ -900,8 +900,8 @@ public class HttpChannelState if (_outputState != OutputState.OPEN) throw new IllegalStateException("Response is " + _outputState); - response.getHttpOutput().closedBySendError(); response.setStatus(code); + response.closedBySendError(); request.setAttribute(ErrorHandler.ERROR_CONTEXT, request.getErrorContext()); request.setAttribute(ERROR_REQUEST_URI, request.getRequestURI()); diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 1ef97d7286e..60b5b10e8b6 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -18,7 +18,6 @@ package org.eclipse.jetty.server; -import java.io.Closeable; import java.io.IOException; import java.io.PrintWriter; import java.nio.channels.IllegalSelectorException; @@ -28,7 +27,6 @@ import java.util.EnumSet; import java.util.Iterator; import java.util.ListIterator; import java.util.Locale; -import java.util.concurrent.atomic.AtomicInteger; import java.util.function.Supplier; import javax.servlet.ServletOutputStream; import javax.servlet.ServletResponse; @@ -58,6 +56,7 @@ import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.http.PreEncodedHttpField; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.server.session.SessionHandler; +import org.eclipse.jetty.util.AtomicBiInteger; import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; @@ -87,7 +86,7 @@ public class Response implements HttpServletResponse private final HttpChannel _channel; private final HttpFields _fields = new HttpFields(); - private final AtomicInteger _include = new AtomicInteger(); + private final AtomicBiInteger _errorSentAndIncludes = new AtomicBiInteger(); // hi is errorSent flag, lo is include count private final HttpOutput _out; private int _status = HttpStatus.OK_200; private String _reason; @@ -140,19 +139,44 @@ public class Response implements HttpServletResponse return _out; } + public void reopen() + { + setErrorSent(false); + _out.reopen(); + } + + public void closedBySendError() + { + setErrorSent(true); + _out.closedBySendError(); + } + + /** + * @return true if the response is mutable, ie not errorSent and not included + */ + private boolean isMutable() + { + return _errorSentAndIncludes.get() == 0; + } + + private void setErrorSent(boolean errorSent) + { + _errorSentAndIncludes.getAndSetHi(errorSent ? 1 : 0); + } + public boolean isIncluding() { - return _include.get() > 0; + return _errorSentAndIncludes.getLo() > 0; } public void include() { - _include.incrementAndGet(); + _errorSentAndIncludes.add(0, 1); } public void included() { - _include.decrementAndGet(); + _errorSentAndIncludes.add(0, -1); if (_outputType == OutputType.WRITER) { _writer.reopen(); @@ -438,7 +462,7 @@ public class Response implements HttpServletResponse if ((code < HttpServletResponse.SC_MULTIPLE_CHOICES) || (code >= HttpServletResponse.SC_BAD_REQUEST)) throw new IllegalArgumentException("Not a 3xx redirect code"); - if (isIncluding()) + if (!isMutable()) return; if (location == null) @@ -484,34 +508,34 @@ public class Response implements HttpServletResponse @Override public void setDateHeader(String name, long date) { - if (!isIncluding()) + if (isMutable()) _fields.putDateField(name, date); } @Override public void addDateHeader(String name, long date) { - if (!isIncluding()) + if (isMutable()) _fields.addDateField(name, date); } public void setHeader(HttpHeader name, String value) { - if (HttpHeader.CONTENT_TYPE == name) - setContentType(value); - else + if (isMutable()) { - if (isIncluding()) - return; - - _fields.put(name, value); - - if (HttpHeader.CONTENT_LENGTH == name) + if (HttpHeader.CONTENT_TYPE == name) + setContentType(value); + else { - if (value == null) - _contentLength = -1L; - else - _contentLength = Long.parseLong(value); + _fields.put(name, value); + + if (HttpHeader.CONTENT_LENGTH == name) + { + if (value == null) + _contentLength = -1L; + else + _contentLength = Long.parseLong(value); + } } } } @@ -519,17 +543,21 @@ public class Response implements HttpServletResponse @Override public void setHeader(String name, String value) { + long biInt = _errorSentAndIncludes.get(); + if (biInt != 0) + { + boolean errorSent = AtomicBiInteger.getHi(biInt) != 0; + boolean including = AtomicBiInteger.getLo(biInt) > 0; + if (!errorSent && including && name.startsWith(SET_INCLUDE_HEADER_PREFIX)) + name = name.substring(SET_INCLUDE_HEADER_PREFIX.length()); + else + return; + } + if (HttpHeader.CONTENT_TYPE.is(name)) setContentType(value); else { - if (isIncluding()) - { - if (name.startsWith(SET_INCLUDE_HEADER_PREFIX)) - name = name.substring(SET_INCLUDE_HEADER_PREFIX.length()); - else - return; - } _fields.put(name, value); if (HttpHeader.CONTENT_LENGTH.is(name)) { @@ -565,9 +593,12 @@ public class Response implements HttpServletResponse @Override public void addHeader(String name, String value) { - if (isIncluding()) + long biInt = _errorSentAndIncludes.get(); + if (biInt != 0) { - if (name.startsWith(SET_INCLUDE_HEADER_PREFIX)) + boolean errorSent = AtomicBiInteger.getHi(biInt) != 0; + boolean including = AtomicBiInteger.getLo(biInt) > 0; + if (!errorSent && including && name.startsWith(SET_INCLUDE_HEADER_PREFIX)) name = name.substring(SET_INCLUDE_HEADER_PREFIX.length()); else return; @@ -591,7 +622,7 @@ public class Response implements HttpServletResponse @Override public void setIntHeader(String name, int value) { - if (!isIncluding()) + if (isMutable()) { _fields.putLongField(name, value); if (HttpHeader.CONTENT_LENGTH.is(name)) @@ -602,7 +633,7 @@ public class Response implements HttpServletResponse @Override public void addIntHeader(String name, int value) { - if (!isIncluding()) + if (isMutable()) { _fields.add(name, Integer.toString(value)); if (HttpHeader.CONTENT_LENGTH.is(name)) @@ -615,7 +646,7 @@ public class Response implements HttpServletResponse { if (sc <= 0) throw new IllegalArgumentException(); - if (!isIncluding()) + if (isMutable()) { // Null the reason only if the status is different. This allows // a specific reason to be sent with setStatusWithReason followed by sendError. @@ -636,7 +667,7 @@ public class Response implements HttpServletResponse { if (sc <= 0) throw new IllegalArgumentException(); - if (!isIncluding()) + if (isMutable()) { _status = sc; _reason = sm; @@ -742,7 +773,7 @@ public class Response implements HttpServletResponse // Protect from setting after committed as default handling // of a servlet HEAD request ALWAYS sets _content length, even // if the getHandling committed the response! - if (isCommitted() || isIncluding()) + if (isCommitted() || !isMutable()) return; if (len > 0) @@ -818,7 +849,7 @@ public class Response implements HttpServletResponse // Protect from setting after committed as default handling // of a servlet HEAD request ALWAYS sets _content length, even // if the getHandling committed the response! - if (isCommitted() || isIncluding()) + if (isCommitted() || !isMutable()) return; _contentLength = len; _fields.putLongField(HttpHeader.CONTENT_LENGTH.toString(), len); @@ -838,7 +869,7 @@ public class Response implements HttpServletResponse private void setCharacterEncoding(String encoding, EncodingFrom from) { - if (isIncluding() || isWriting()) + if (!isMutable() || isWriting()) return; if (_outputType != OutputType.WRITER && !isCommitted()) @@ -891,7 +922,7 @@ public class Response implements HttpServletResponse @Override public void setContentType(String contentType) { - if (isCommitted() || isIncluding()) + if (isCommitted() || !isMutable()) return; if (contentType == null) @@ -1146,7 +1177,7 @@ public class Response implements HttpServletResponse @Override public void setLocale(Locale locale) { - if (locale == null || isCommitted() || isIncluding()) + if (locale == null || isCommitted() || !isMutable()) return; _locale = locale; diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java index 7e396b072b2..1216bc06ac0 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ErrorHandlerTest.java @@ -53,32 +53,6 @@ public class ErrorHandlerTest connector = new LocalConnector(server); server.addConnector(connector); - server.setHandler(new AbstractHandler() - { - @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException - { - if (baseRequest.getDispatcherType() == DispatcherType.ERROR) - { - baseRequest.setHandled(true); - response.sendError(((Integer)request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE)).intValue()); - return; - } - - if (target.startsWith("/charencoding/")) - { - baseRequest.setHandled(true); - response.setCharacterEncoding("utf-8"); - response.sendError(404); - return; - } - - if (target.startsWith("/badmessage/")) - { - throw new ServletException(new BadMessageException(Integer.valueOf(target.substring(12)))); - } - } - }); server.setHandler(new AbstractHandler() { @Override diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java index 4cb4f63a48a..ac4bee7c9bc 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseTest.java @@ -689,9 +689,14 @@ public class ResponseTest else response.sendError(code, message); + assertTrue(response.getHttpOutput().isClosed()); assertEquals(code, response.getStatus()); assertEquals(null, response.getReason()); + + response.setHeader("Should-Be-Ignored", "value"); + assertFalse(response.getHttpFields().containsKey("Should-Be-Ignored")); + assertEquals(expectedMessage, response.getHttpChannel().getRequest().getAttribute(RequestDispatcher.ERROR_MESSAGE)); assertThat(response.getHttpChannel().getState().unhandle(), is(HttpChannelState.Action.SEND_ERROR)); assertThat(response.getHttpChannel().getState().unhandle(), is(HttpChannelState.Action.COMPLETE));