From 71e7f519c7e07f5aa4942dd6e9281b4dcdf6b229 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Tue, 21 Aug 2012 23:40:08 +0200 Subject: [PATCH] Jetty9 - Refactored error handling logic. --- .../org/eclipse/jetty/http/HttpGenerator.java | 10 +- .../org/eclipse/jetty/server/HttpChannel.java | 176 ++++++++++++------ .../eclipse/jetty/server/HttpConnection.java | 9 +- .../org/eclipse/jetty/server/HttpOutput.java | 25 +-- .../eclipse/jetty/server/HttpTransport.java | 2 +- .../jetty/server/HttpTransportOverHttp.java | 30 ++- .../org/eclipse/jetty/server/Request.java | 3 +- .../org/eclipse/jetty/server/Response.java | 151 +++++---------- .../server/HttpManyWaysToCommitTest.java | 70 +++---- .../test/resources/jetty-logging.properties | 3 +- 10 files changed, 248 insertions(+), 231 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index 990bc7d21ae..a9c719a8377 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -384,11 +384,10 @@ public class HttpGenerator if (len>0) { // Do we need a chunk buffer? - if (isChunking() && (headerOrChunk==null || headerOrChunk.capacity()>CHUNK_SIZE)) + if (isChunking() && BufferUtil.space(headerOrChunk) < CHUNK_SIZE) return Result.NEED_CHUNK; ByteBuffer chunk = headerOrChunk; - _contentPrepared+=len; if (isChunking()) { @@ -421,11 +420,11 @@ public class HttpGenerator if (isChunking()) { // Do we need a chunk buffer? - if (headerOrChunk==null || headerOrChunk.capacity()>CHUNK_SIZE) + if (BufferUtil.space(headerOrChunk) < CHUNK_SIZE) return Result.NEED_CHUNK; - ByteBuffer chunk=headerOrChunk; // Write the last chunk + ByteBuffer chunk=headerOrChunk; BufferUtil.clearToFill(chunk); prepareChunk(chunk,0); BufferUtil.flipToFlush(chunk,0); @@ -448,8 +447,6 @@ public class HttpGenerator } } - - /* ------------------------------------------------------------ */ private void prepareChunk(ByteBuffer chunk, int remaining) { @@ -471,7 +468,6 @@ public class HttpGenerator } } - /* ------------------------------------------------------------ */ private void generateRequestLine(RequestInfo request,ByteBuffer header) { 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 278fd2d8c91..c11f7a74e68 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 @@ -24,8 +24,10 @@ import java.nio.ByteBuffer; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.DispatcherType; +import javax.servlet.RequestDispatcher; import org.eclipse.jetty.continuation.ContinuationThrowable; +import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpGenerator; import org.eclipse.jetty.http.HttpGenerator.ResponseInfo; import org.eclipse.jetty.http.HttpHeader; @@ -38,6 +40,8 @@ import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MimeTypes; import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.io.EofException; +import org.eclipse.jetty.server.handler.ContextHandler; +import org.eclipse.jetty.server.handler.ErrorHandler; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; @@ -170,7 +174,7 @@ public class HttpChannel throw new IOException("Committed before 100 Continues"); // TODO: break this dependency with HttpGenerator - boolean committed = commit(HttpGenerator.CONTINUE_100_INFO, null, false); + boolean committed = commitResponse(HttpGenerator.CONTINUE_100_INFO, null, false); if (!committed) throw new IOException("Concurrent commit while trying to send 100-Continue"); // TODO: better message } @@ -243,7 +247,7 @@ public class HttpChannel LOG.warn(String.valueOf(_uri), e); _state.error(e); _request.setHandled(true); - commitError(500, null, e.toString()); + handleError(e); } finally { @@ -251,7 +255,7 @@ public class HttpChannel } } - return complete1(); + return complete(); } finally { @@ -264,7 +268,7 @@ public class HttpChannel } } - protected boolean complete1() + protected boolean complete() { LOG.debug("{} complete", this); @@ -287,7 +291,7 @@ public class HttpChannel } if (!_response.isCommitted() && !_request.isHandled()) - commitError(404, null, null); // TODO: this should call the ErrorHandler + _response.sendError(Response.SC_NOT_FOUND, null, null); _request.setHandled(true); _request.getHttpInput().consumeAll(); @@ -380,62 +384,130 @@ public class HttpChannel */ } - protected boolean commitError(final int status, final String reason, String content) + /** + *

Sends an error 500, performing a special logic to detect whether the request is suspended, + * to avoid concurrent writes from the application.

+ *

It may happen that the application suspends, and then throws an exception, while an application + * spawned thread writes the response content; in such case, we attempt to commit the error directly + * bypassing the {@link ErrorHandler} mechanisms and the response OutputStream.

+ * @param x the Throwable that caused the problem + */ + private void handleError(Throwable x) { - return true; // TODO -/* - LOG.debug("{} sendError {} {}", this, status, reason); - - if (_response.isCommitted()) - return false; - - try + if (_state.isSuspended()) { - _response.setStatus(status, reason); - _response.getHttpFields().add(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE); - - ByteBuffer buffer = null; - if (content != null) - { - buffer = BufferUtil.toBuffer(content, StringUtil.__UTF8_CHARSET); - _response.setContentLength(buffer.remaining()); - } - - HttpGenerator.ResponseInfo info = _handler.commit(); try { - write(info, buffer).get(); + HttpFields fields = new HttpFields(); + ResponseInfo info = new ResponseInfo(_request.getHttpVersion(), fields, 0, Response.SC_INTERNAL_SERVER_ERROR, null, _request.isHead()); + boolean committed = commitResponse(info, null, true); + if (!committed) + LOG.warn("Could not send response error 500, response is already committed"); } - catch (final InterruptedException e) + catch (IOException e) { - throw new InterruptedIOException() - {{ - this.initCause(e); - }}; + // We tried our best, just log + LOG.debug("Could not commit response error 500", e); } - catch (final ExecutionException e) + } + else + { + _response.sendError(500, null, x.getMessage()); + } + } + + protected void sendError(ResponseInfo info, String extraContent) + { + int status = info.getStatus(); + try + { + String reason = info.getReason(); + if (reason == null) + reason = HttpStatus.getMessage(status); + + // If we are allowed to have a body + if (status != Response.SC_NO_CONTENT && + status != Response.SC_NOT_MODIFIED && + status != Response.SC_PARTIAL_CONTENT && + status >= Response.SC_OK) { - throw new IOException() - {{ - this.initCause(e); - }}; + ErrorHandler errorHandler = null; + ContextHandler.Context context = _request.getContext(); + if (context != null) + errorHandler = context.getContextHandler().getErrorHandler(); + if (errorHandler == null) + errorHandler = getServer().getBean(ErrorHandler.class); + if (errorHandler != null) + { + _request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE, new Integer(status)); + _request.setAttribute(RequestDispatcher.ERROR_MESSAGE, reason); + _request.setAttribute(RequestDispatcher.ERROR_REQUEST_URI, _request.getRequestURI()); + _request.setAttribute(RequestDispatcher.ERROR_SERVLET_NAME, _request.getServletName()); + errorHandler.handle(null, _request, _request, _response); + } + else + { + HttpFields fields = info.getHttpFields(); + fields.put(HttpHeader.CACHE_CONTROL, "must-revalidate,no-cache,no-store"); + fields.put(HttpHeader.CONTENT_TYPE, MimeTypes.Type.TEXT_HTML_8859_1.toString()); + + reason = escape(reason); + String uri = escape(_request.getRequestURI()); + extraContent = escape(extraContent); + + StringBuilder writer = new StringBuilder(2048); + writer.append("\n"); + writer.append("\n"); + writer.append("\n"); + writer.append("Error ").append(Integer.toString(status)).append(' ').append(reason).append("\n"); + writer.append("\n"); + writer.append("\n"); + writer.append("

HTTP ERROR: ").append(Integer.toString(status)).append("

\n"); + writer.append("

Problem accessing ").append(uri).append(". Reason:\n"); + writer.append("

").append(reason).append("

"); + if (extraContent != null) + writer.append("

").append(extraContent).append("

"); + writer.append("
Powered by Jetty://\n"); + writer.append("\n"); + writer.append(""); + byte[] bytes = writer.toString().getBytes(StringUtil.__ISO_8859_1); + fields.put(HttpHeader.CONTENT_LENGTH, String.valueOf(bytes.length)); + _response.getOutputStream().write(bytes); + } + } + else if (status != Response.SC_PARTIAL_CONTENT) + { + // TODO: not sure why we need to modify the request when writing an error ? + // TODO: or modify the response if the error code cannot have a body ? + // _channel.getRequest().getHttpFields().remove(HttpHeader.CONTENT_TYPE); + // _channel.getRequest().getHttpFields().remove(HttpHeader.CONTENT_LENGTH); + // _characterEncoding = null; + // _mimeType = null; } - return true; - } - catch (Exception e) - { - e.printStackTrace(); - LOG.debug("failed to sendError {} {}", status, reason, e); - } - finally - { + complete(); + + // TODO: is this needed ? if (_state.isIdle()) _state.complete(); _request.getHttpInput().shutdownInput(); } - return false; -*/ + catch (IOException x) + { + // We failed to write the error, bail out + LOG.debug("Could not write error response " + status, x); + } + } + + private String escape(String reason) + { + if (reason != null) + { + reason = reason.replaceAll("&", "&"); + reason = reason.replaceAll("<", "<"); + reason = reason.replaceAll(">", ">"); + } + return reason; } public boolean isSuspended() @@ -604,13 +676,13 @@ public class HttpChannel if (!_host) { - commitError(HttpStatus.BAD_REQUEST_400, "No Host Header", null); + _response.sendError(Response.SC_BAD_REQUEST, "No Host Header", null); return true; } if (_expect) { - commitError(HttpStatus.EXPECTATION_FAILED_417, null, null); + _response.sendError(Response.SC_EXPECTATION_FAILED, null, null); return true; } @@ -658,7 +730,7 @@ public class HttpChannel { if (status < 400 || status > 599) status = HttpStatus.BAD_REQUEST_400; - commitError(status, null, null); + _response.sendError(status, null, null); } @Override @@ -680,7 +752,7 @@ public class HttpChannel } } - protected boolean commit(ResponseInfo info, ByteBuffer content, boolean complete) throws IOException + protected boolean commitResponse(ResponseInfo info, ByteBuffer content, boolean complete) throws IOException { boolean committed = _committed.compareAndSet(false, true); if (committed) @@ -710,7 +782,7 @@ public class HttpChannel else { ResponseInfo info = _response.newResponseInfo(); - boolean committed = commit(info, content, complete); + boolean committed = commitResponse(info, content, complete); if (!committed) throw new IOException("Concurrent commit"); // TODO: better message } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java index e3ef42f0d30..b03907499a7 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnection.java @@ -337,7 +337,8 @@ public class HttpConnection extends AbstractConnection { _parser.reset(); _parser.close(); - _channel.getEventHandler().badMessage(HttpStatus.REQUEST_ENTITY_TOO_LARGE_413,null); + _channel.getResponse().sendError(Response.SC_REQUEST_ENTITY_TOO_LARGE, null, null); + // TODO: close the connection ! } } } @@ -397,10 +398,10 @@ public class HttpConnection extends AbstractConnection return _httpConfig; } - @Override +// @Override protected boolean commitError(int status, String reason, String content) { - if (!super.commitError(status,reason,content)) +// if (!super.commitError(status,reason,content)) { // TODO - should this just be a close and we don't worry about a RST overtaking a flushed response? @@ -408,7 +409,7 @@ public class HttpConnection extends AbstractConnection // the client something is wrong getEndPoint().shutdownOutput(); _generator.abort(); - return false; +// return false; } return true; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java index 1c64ae1afa3..eeadf2ed6af 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpOutput.java @@ -49,10 +49,12 @@ public class HttpOutput extends ServletOutputStream private boolean _closed; private long _written; private ByteBuffer _aggregate; + private int _bufferSize; public HttpOutput(HttpChannel channel) { _channel = channel; + _bufferSize = _channel.getHttpConfiguration().getResponseBufferSize(); } public boolean isWritten() @@ -126,7 +128,7 @@ public class HttpOutput extends ServletOutputStream if (_aggregate == null) { // What size should the aggregate be ? - int size = _channel.getHttpConfiguration().getResponseBufferSize(); + int size = getBufferSize(); // If this write would fill more than half the aggregate, just write it directly if (len > size / 2) @@ -176,7 +178,7 @@ public class HttpOutput extends ServletOutputStream throw new EOFException(); if (_aggregate == null) - _aggregate = _channel.getConnector().getByteBufferPool().acquire(_channel.getHttpConfiguration().getResponseBufferSize(), false); + _aggregate = _channel.getConnector().getByteBufferPool().acquire(getBufferSize(), false); BufferUtil.append(_aggregate, (byte)b); _written++; @@ -199,7 +201,6 @@ public class HttpOutput extends ServletOutputStream if (isClosed()) throw new IOException("Closed"); - // Convert HTTP content to contentl if (content instanceof HttpContent) { HttpContent httpContent = (HttpContent)content; @@ -247,24 +248,14 @@ public class HttpOutput extends ServletOutputStream throw new IllegalArgumentException("unknown content type?"); } - public int getContentBufferSize() + public int getBufferSize() { - if (_aggregate != null) - return _aggregate.capacity(); - return _channel.getHttpConfiguration().getResponseBufferSize(); + return _bufferSize; } - public void increaseContentBufferSize(int size) + public void setBufferSize(int size) { - if (_aggregate == null || size <= getContentBufferSize()) - return; - - ByteBuffer r = _channel.getConnector().getByteBufferPool().acquire(size, false); - if (BufferUtil.hasContent(_aggregate)) - BufferUtil.flipPutFlip(_aggregate, r); - if (_aggregate != null) - _channel.getConnector().getByteBufferPool().release(_aggregate); - _aggregate = r; + this._bufferSize = size; } public void resetBuffer() diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransport.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransport.java index 13094793eb9..eff654753fe 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransport.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransport.java @@ -9,5 +9,5 @@ public interface HttpTransport { public void commit(HttpGenerator.ResponseInfo info, ByteBuffer content, boolean complete) throws IOException; - public int write(ByteBuffer content, boolean complete) throws IOException; + public void write(ByteBuffer content, boolean complete) throws IOException; } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransportOverHttp.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransportOverHttp.java index 9a1d2aa5609..66e29715b8a 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransportOverHttp.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpTransportOverHttp.java @@ -38,6 +38,7 @@ public class HttpTransportOverHttp implements HttpTransport private final ByteBufferPool _bufferPool; private final HttpConfiguration _configuration; private final EndPoint _endPoint; + private HttpGenerator.ResponseInfo _info; public HttpTransportOverHttp(ByteBufferPool _bufferPool, HttpConfiguration _configuration, EndPoint _endPoint) { @@ -48,6 +49,23 @@ public class HttpTransportOverHttp implements HttpTransport @Override public void commit(HttpGenerator.ResponseInfo info, ByteBuffer content, boolean complete) throws IOException + { + generate(info, content, complete); + // TODO: Trick only needed by the current HttpGenerator, that always require a ResponseInfo object + if (!complete) + _info = info; + } + + @Override + public void write(ByteBuffer content, boolean complete) throws IOException + { + generate(_info, content, complete); + // TODO: Trick only needed by the current HttpGenerator, that always require a ResponseInfo object + if (complete) + _info = null; + } + + private void generate(HttpGenerator.ResponseInfo info, ByteBuffer content, boolean complete) throws IOException { ByteBuffer header = null; out: while (true) @@ -87,7 +105,7 @@ public class HttpTransportOverHttp implements HttpTransport } else { - write(header, content); + write(header == null ? BufferUtil.EMPTY_BUFFER : header, content); } continue; } @@ -100,6 +118,10 @@ public class HttpTransportOverHttp implements HttpTransport { break out; } + case CONTINUE: + { + break; + } default: { throw new IllegalStateException(); @@ -131,10 +153,4 @@ public class HttpTransportOverHttp implements HttpTransport throw (Error)cause; } } - - @Override - public int write(ByteBuffer content, boolean complete) throws IOException - { - throw new UnsupportedOperationException(); - } } diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 278b475008e..e24bc9a19bc 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -1081,8 +1081,9 @@ public class Request implements HttpServletRequest } catch (NumberFormatException e) { + // TODO: this should be moved to the parser ! if (_channel != null) - _channel.commitError(HttpStatus.BAD_REQUEST_400,"Bad Host header",null); + getResponse().sendError(Response.SC_BAD_REQUEST, "Bad Host header", null); } return _serverName; } 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 9959c4419ab..19d9696fe9f 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 @@ -26,7 +26,6 @@ import java.util.Collections; import java.util.Locale; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicInteger; -import javax.servlet.RequestDispatcher; import javax.servlet.ServletOutputStream; import javax.servlet.http.Cookie; import javax.servlet.http.HttpServletResponse; @@ -43,9 +42,6 @@ import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.MimeTypes; -import org.eclipse.jetty.server.handler.ContextHandler; -import org.eclipse.jetty.server.handler.ErrorHandler; -import org.eclipse.jetty.util.ByteArrayISO8859Writer; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.URIUtil; import org.eclipse.jetty.util.log.Log; @@ -60,7 +56,10 @@ public class Response implements HttpServletResponse { private static final Logger LOG = Log.getLogger(Response.class); - public enum OutputState {NONE,STREAM,WRITER} + public enum OutputType + { + NONE,STREAM,WRITER + } /** * If a header name starts with this string, the header (stripped of the prefix) @@ -86,7 +85,7 @@ public class Response implements HttpServletResponse private MimeTypes.Type _mimeType; private String _characterEncoding; private String _contentType; - private OutputState _outputState=OutputState.NONE; + private OutputType _outputType = OutputType.NONE; private PrintWriter _writer; private long _contentLength=-1; @@ -119,7 +118,7 @@ public class Response implements HttpServletResponse _characterEncoding=null; _contentType=null; _writer=null; - _outputState=OutputState.NONE; + _outputType = OutputType.NONE; _contentLength=-1; _committed.set(false); _out.reset(); @@ -334,99 +333,35 @@ public class Response implements HttpServletResponse @Override public void sendError(int code, String message) throws IOException { - if (isIncluding()) - return; + sendError(code, message, null); + } + + protected void sendError(int code, String message, String content) + { + if (isIncluding()) + return; if (isCommitted()) - LOG.warn("Committed before "+code+" "+message); + { + LOG.warn("Could not commit error {} {}, response already committed", code, message); + return; + } resetBuffer(); - _characterEncoding=null; - setHeader(HttpHeader.EXPIRES,null); - setHeader(HttpHeader.LAST_MODIFIED,null); - setHeader(HttpHeader.CACHE_CONTROL,null); - setHeader(HttpHeader.CONTENT_TYPE,null); - setHeader(HttpHeader.CONTENT_LENGTH,null); + _characterEncoding = null; + setHeader(HttpHeader.EXPIRES, null); + setHeader(HttpHeader.LAST_MODIFIED, null); + setHeader(HttpHeader.CACHE_CONTROL, null); + setHeader(HttpHeader.CONTENT_TYPE, null); + setHeader(HttpHeader.CONTENT_LENGTH, null); - _outputState=OutputState.NONE; - setStatus(code,message); + if (message == null) + message = HttpStatus.getMessage(code); + setStatus(code, message); - if (message==null) - message=HttpStatus.getMessage(code); + _outputType = OutputType.NONE; - // If we are allowed to have a body - if (code!=SC_NO_CONTENT && - code!=SC_NOT_MODIFIED && - code!=SC_PARTIAL_CONTENT && - code>=SC_OK) - { - Request request = _channel.getRequest(); - - ErrorHandler error_handler = null; - ContextHandler.Context context = request.getContext(); - if (context!=null) - error_handler=context.getContextHandler().getErrorHandler(); - if (error_handler==null) - error_handler = _channel.getServer().getBean(ErrorHandler.class); - if (error_handler!=null) - { - request.setAttribute(RequestDispatcher.ERROR_STATUS_CODE,new Integer(code)); - request.setAttribute(RequestDispatcher.ERROR_MESSAGE, message); - request.setAttribute(RequestDispatcher.ERROR_REQUEST_URI, request.getRequestURI()); - request.setAttribute(RequestDispatcher.ERROR_SERVLET_NAME,request.getServletName()); - error_handler.handle(null,_channel.getRequest(),_channel.getRequest(),this ); - } - else - { - setHeader(HttpHeader.CACHE_CONTROL, "must-revalidate,no-cache,no-store"); - setContentType(MimeTypes.Type.TEXT_HTML_8859_1.toString()); - ByteArrayISO8859Writer writer= new ByteArrayISO8859Writer(2048); - if (message != null) - { - message= StringUtil.replace(message, "&", "&"); - message= StringUtil.replace(message, "<", "<"); - message= StringUtil.replace(message, ">", ">"); - } - String uri= request.getRequestURI(); - if (uri!=null) - { - uri= StringUtil.replace(uri, "&", "&"); - uri= StringUtil.replace(uri, "<", "<"); - uri= StringUtil.replace(uri, ">", ">"); - } - - writer.write("\n\n\n"); - writer.write("Error "); - writer.write(Integer.toString(code)); - writer.write(' '); - if (message==null) - message=HttpStatus.getMessage(code); - writer.write(message); - writer.write("\n\n\n

HTTP ERROR: "); - writer.write(Integer.toString(code)); - writer.write("

\n

Problem accessing "); - writer.write(uri); - writer.write(". Reason:\n

    ");
-                writer.write(message);
-                writer.write("
"); - writer.write("

\n
Powered by Jetty://"); - writer.write("\n\n\n"); - - writer.flush(); - setContentLength(writer.size()); - writer.writeTo(getOutputStream()); - writer.destroy(); - } - } - else if (code!=SC_PARTIAL_CONTENT) - { - _channel.getRequest().getHttpFields().remove(HttpHeader.CONTENT_TYPE); - _channel.getRequest().getHttpFields().remove(HttpHeader.CONTENT_LENGTH); - _characterEncoding=null; - _mimeType=null; - } - - complete(); + _channel.sendError(newResponseInfo(), content); } /* ------------------------------------------------------------ */ @@ -454,7 +389,7 @@ public class Response implements HttpServletResponse { if (_channel.isExpecting102Processing() && !isCommitted()) { - _channel.commit(HttpGenerator.PROGRESS_102_INFO,null,true); + _channel.commitResponse(HttpGenerator.PROGRESS_102_INFO, null, true); } } @@ -760,22 +695,22 @@ public class Response implements HttpServletResponse @Override public ServletOutputStream getOutputStream() throws IOException { - if (_outputState==OutputState.WRITER) + if (_outputType == OutputType.WRITER) throw new IllegalStateException("WRITER"); - _outputState=OutputState.STREAM; + _outputType = OutputType.STREAM; return _out; } /* ------------------------------------------------------------ */ public boolean isWriting() { - return _outputState==OutputState.WRITER; + return _outputType == OutputType.WRITER; } /* ------------------------------------------------------------ */ public boolean isOutputing() { - return _outputState!=OutputState.NONE; + return _outputType != OutputType.NONE; } /* ------------------------------------------------------------ */ @@ -785,7 +720,7 @@ public class Response implements HttpServletResponse @Override public PrintWriter getWriter() throws IOException { - if (_outputState==OutputState.STREAM) + if (_outputType == OutputType.STREAM) throw new IllegalStateException("STREAM"); /* if there is no writer yet */ @@ -817,7 +752,7 @@ public class Response implements HttpServletResponse } } - _outputState=OutputState.WRITER; + _outputType = OutputType.WRITER; return _writer; } @@ -854,7 +789,7 @@ public class Response implements HttpServletResponse { try { - switch(_outputState) + switch(_outputType) { case WRITER: _writer.close(); @@ -904,7 +839,7 @@ public class Response implements HttpServletResponse if (isIncluding()) return; - if (_outputState==OutputState.NONE && !isCommitted()) + if (_outputType == OutputType.NONE && !isCommitted()) { if (encoding==null) { @@ -1010,7 +945,7 @@ public class Response implements HttpServletResponse { if (isCommitted() || getContentCount()>0 ) throw new IllegalStateException("Committed or content written"); - _out.increaseContentBufferSize(size); + _out.setBufferSize(size); } /* ------------------------------------------------------------ */ @@ -1020,7 +955,7 @@ public class Response implements HttpServletResponse @Override public int getBufferSize() { - return _out.getContentBufferSize(); + return _out.getBufferSize(); } /* ------------------------------------------------------------ */ @@ -1087,7 +1022,7 @@ public class Response implements HttpServletResponse resetBuffer(); _writer=null; - _outputState=OutputState.NONE; + _outputType = OutputType.NONE; } /* ------------------------------------------------------------ */ @@ -1100,7 +1035,7 @@ public class Response implements HttpServletResponse if (isCommitted()) throw new IllegalStateException("Committed"); - switch(_outputState) + switch(_outputType) { case STREAM: case WRITER: @@ -1134,7 +1069,7 @@ public class Response implements HttpServletResponse @Override public boolean isCommitted() { - return _committed.get(); + return _channel.isCommitted(); } /* ------------------------------------------------------------ */ @@ -1150,7 +1085,7 @@ public class Response implements HttpServletResponse _locale = locale; _fields.put(HttpHeader.CONTENT_LANGUAGE,locale.toString().replace('_','-')); - if (_outputState!=OutputState.NONE ) + if (_outputType != OutputType.NONE ) return; if (_channel.getRequest().getContext()==null) diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToCommitTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToCommitTest.java index 87b73df5706..a3d272e531e 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToCommitTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpManyWaysToCommitTest.java @@ -27,13 +27,12 @@ import javax.servlet.http.HttpServletResponse; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.server.util.SimpleHttpParser; -import org.eclipse.jetty.util.log.Log; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertThat; //TODO: reset buffer tests @@ -113,7 +112,6 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest assertThat("response code is 500", response.getCode(), is("500")); } - private class OnlySetHandledHandler extends ThrowExceptionOnDemandHandler { private OnlySetHandledHandler(boolean throwException) @@ -138,6 +136,7 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); assertThat("response code is 200", response.getCode(), is("200")); + assertResponseBody(response, "foobar"); assertHeader(response, "content-length", "6"); } @@ -150,6 +149,7 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); assertThat("response code is 500", response.getCode(), is("500")); + assertThat("response body is not foobar", response.getBody(), not(is("foobar"))); } private class SetHandledWriteSomeDataHandler extends ThrowExceptionOnDemandHandler @@ -176,8 +176,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); - assertThat("response code is 200", response.getCode(), is("200")); + assertResponseBody(response, "foobar"); if ("HTTP/1.1".equals(httpVersion)) assertHeader(response, "transfer-encoding", "chunked"); } @@ -190,7 +190,9 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); + // Since the 200 was committed, the 500 did not get the chance to be written assertThat("response code is 200", response.getCode(), is("200")); + assertThat("response body is foobar", response.getBody(), is("foobar")); if ("HTTP/1.1".equals(httpVersion)) assertHeader(response, "transfer-encoding", "chunked"); } @@ -255,7 +257,7 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest } @Test - public void testWriteFlushWriteMore() throws Exception + public void testHandledWriteFlushWriteMore() throws Exception { server.setHandler(new WriteFlushWriteMoreHandler(false)); server.start(); @@ -263,21 +265,24 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); assertThat("response code is 200", response.getCode(), is("200")); + assertResponseBody(response, "foobar"); if ("HTTP/1.1".equals(httpVersion)) - assertHeader(response, "transfer-encoding", "chunked"); // HTTP/1.0 does not do chunked. it will just send content and close + assertHeader(response, "transfer-encoding", "chunked"); } @Test - public void testWriteFlushWriteMoreAndThrow() throws Exception + public void testHandledWriteFlushWriteMoreAndThrow() throws Exception { server.setHandler(new WriteFlushWriteMoreHandler(true)); server.start(); SimpleHttpParser.TestHttpResponse response = executeRequest(); + // Since the 200 was committed, the 500 did not get the chance to be written + assertThat("response code is 200", response.getCode(), is("200")); assertThat("response code is 200", response.getCode(), is("200")); if ("HTTP/1.1".equals(httpVersion)) - assertHeader(response, "transfer-encoding", "chunked"); // TODO HTTP/1.0 does not do chunked + assertHeader(response, "transfer-encoding", "chunked"); } private class WriteFlushWriteMoreHandler extends ThrowExceptionOnDemandHandler @@ -299,7 +304,7 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest } @Test - public void testBufferOverflow() throws Exception + public void testHandledBufferOverflow() throws Exception { server.setHandler(new OverflowHandler(false)); server.start(); @@ -308,19 +313,23 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest assertThat("response code is 200", response.getCode(), is("200")); assertResponseBody(response, "foobar"); - assertHeader(response, "content-length", "6"); + if ("HTTP/1.1".equals(httpVersion)) + assertHeader(response, "transfer-encoding", "chunked"); } @Test - public void testBufferOverflowAndThrow() throws Exception + public void testHandledBufferOverflowAndThrow() throws Exception { server.setHandler(new OverflowHandler(true)); server.start(); SimpleHttpParser.TestHttpResponse response = executeRequest(); - // response not committed when we throw, so 500 expected - assertThat("response code is 500", response.getCode(), is("500")); + // Response was committed when we throw, so 200 expected + assertThat("response code is 200", response.getCode(), is("200")); + assertResponseBody(response, "foobar"); + if ("HTTP/1.1".equals(httpVersion)) + assertHeader(response, "transfer-encoding", "chunked"); } private class OverflowHandler extends ThrowExceptionOnDemandHandler @@ -351,7 +360,6 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest assertThat("response code is 200", response.getCode(), is("200")); assertThat("response body is foo", response.getBody(), is("foo")); assertHeader(response, "content-length", "3"); - } @Test @@ -362,10 +370,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); - //TODO: should we expect 500 here? - assertThat("response code is 200", response.getCode(), is("200")); - assertThat("response body is foo", response.getBody(), is("foo")); - assertHeader(response, "content-length", "3"); + assertThat("response code is 500", response.getCode(), is("500")); + assertThat("response body is not foo", response.getBody(), not(is("foo"))); } private class SetContentLengthAndWriteThatAmountOfBytesHandler extends ThrowExceptionOnDemandHandler @@ -394,7 +400,6 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); assertThat("response code is 200", response.getCode(), is("200")); - // jetty truncates the body when content-length is reached.! This is correct and desired behaviour? assertThat("response body is foo", response.getBody(), is("foo")); assertHeader(response, "content-length", "3"); } @@ -407,10 +412,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); - // TODO: we throw before response is committed. should we expect 500? - assertThat("response code is 200", response.getCode(), is("200")); - assertThat("response body is foo", response.getBody(), is("foo")); - assertHeader(response, "content-length", "3"); + assertThat("response code is 500", response.getCode(), is("500")); + assertThat("response body is not foo", response.getBody(), not(is("foo"))); } private class SetContentLengthAndWriteMoreBytesHandler extends ThrowExceptionOnDemandHandler @@ -425,7 +428,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest { baseRequest.setHandled(true); response.setContentLength(3); - response.getWriter().write("foobar"); // Only "foo" will get written and "bar" will be discarded + // Only "foo" will get written and "bar" will be discarded + response.getWriter().write("foobar"); super.handle(target, baseRequest, request, response); } } @@ -439,7 +443,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); assertThat("response code is 200", response.getCode(), is("200")); - //TODO: jetty ignores setContentLength and sends transfer-encoding header. Correct? + assertThat("response body is foo", response.getBody(), is("foo")); + assertHeader(response, "content-length", "3"); } @Test @@ -450,7 +455,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); - assertThat("response code is 200", response.getCode(), is("200")); + assertThat("response code is 500", response.getCode(), is("500")); + assertThat("response body is not foo", response.getBody(), not(is("foo"))); } private class WriteAndSetContentLengthHandler extends ThrowExceptionOnDemandHandler @@ -465,13 +471,12 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest { baseRequest.setHandled(true); response.getWriter().write("foo"); - response.setContentLength(3); // This should commit the response + response.setContentLength(3); super.handle(target, baseRequest, request, response); } } @Test - @Ignore public void testWriteAndSetContentLengthTooSmall() throws Exception { server.setHandler(new WriteAndSetContentLengthTooSmallHandler(false)); @@ -480,10 +485,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); assertThat("response code is 200", response.getCode(), is("200")); - assertResponseBody(response, "foobar"); - // TODO: once flushed setting contentLength is ignored and chunked is used. Correct? - if ("HTTP/1.1".equals(httpVersion)) - assertHeader(response, "transfer-encoding", "chunked"); + assertThat("response body is foo", response.getBody(), is("foo")); + assertHeader(response, "content-length", "3"); } @Test @@ -494,7 +497,8 @@ public class HttpManyWaysToCommitTest extends AbstractHttpTest SimpleHttpParser.TestHttpResponse response = executeRequest(); - assertThat(response.getCode(), is("500")); + assertThat("response code is 500", response.getCode(), is("500")); + assertThat("response body is not foo", response.getBody(), not(is("foo"))); } private class WriteAndSetContentLengthTooSmallHandler extends ThrowExceptionOnDemandHandler diff --git a/jetty-server/src/test/resources/jetty-logging.properties b/jetty-server/src/test/resources/jetty-logging.properties index e5c7ca9cd9b..459fe408408 100644 --- a/jetty-server/src/test/resources/jetty-logging.properties +++ b/jetty-server/src/test/resources/jetty-logging.properties @@ -1,2 +1,3 @@ org.eclipse.jetty.util.log.class=org.eclipse.jetty.util.log.StdErrLog -# org.eclipse.jetty.LEVEL=WARN +#org.eclipse.jetty.LEVEL=DEBUG +org.eclipse.jetty.server.Server.LEVEL=WARN