From 40ed3a013a73c97e1814bf3ce123bfad9c0dc6f8 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 18 May 2012 13:12:38 +0200 Subject: [PATCH] jetty-9 some progress on HTTP error handling but more work needed. See SelectChannelServerTest --- .../org/eclipse/jetty/http/HttpParser.java | 8 +- .../org/eclipse/jetty/io/WriteFlusher.java | 11 +- .../org/eclipse/jetty/server/HttpChannel.java | 24 ++-- .../eclipse/jetty/server/HttpConnection.java | 113 ++++++++++++------ .../org/eclipse/jetty/server/HttpInput.java | 2 +- .../jetty/server/HttpServerTestBase.java | 41 ++++++- .../jetty/server/HttpServerTestFixture.java | 10 +- .../jetty/server/SelectChannelServerTest.java | 14 +++ 8 files changed, 162 insertions(+), 61 deletions(-) diff --git a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index ab7553d5826..a2d33ad3716 100644 --- a/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -869,9 +869,12 @@ public class HttpParser { // process end states if (_state == State.END) + // TODO should we consume white space here? return false; + if (_state == State.CONTENT && _contentPosition == _contentLength) { + // TODO why is this not _state=_persistent?State.END:State.SEEKING_EOF; _state=State.END; if(_handler.messageComplete(_contentPosition)) return true; @@ -1068,7 +1071,7 @@ public class HttpParser } catch(Exception e) { - LOG.debug(e); + LOG.warn(e); _handler.badMessage(e.toString()); return true; } @@ -1118,7 +1121,8 @@ public class HttpParser public void reset() { // reset state - _state=_persistent?State.START:_state==State.END?State.END:State.SEEKING_EOF; + // TODO why is this not _state=_persistent?State.START:(_state=State.SEEKING_EOF); + _state=_persistent?State.START:(_state==State.END?State.END:State.SEEKING_EOF); _endOfContent=EndOfContent.UNKNOWN_CONTENT; _contentPosition=0; _responseStatus=0; diff --git a/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java b/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java index 9df466889d8..adb33d531eb 100644 --- a/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java +++ b/jetty-io/src/main/java/org/eclipse/jetty/io/WriteFlusher.java @@ -57,17 +57,18 @@ abstract public class WriteFlusher return; } } - - if (!_writing.compareAndSet(true,false)) - throw new ConcurrentModificationException(); - callback.completed(context); } catch (IOException e) { if (!_writing.compareAndSet(true,false)) - throw new ConcurrentModificationException(); + throw new ConcurrentModificationException(e); callback.failed(context,e); + return; } + + if (!_writing.compareAndSet(true,false)) + throw new ConcurrentModificationException(); + callback.completed(context); } /* ------------------------------------------------------------ */ 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 a90e488bb06..88edadc4854 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 @@ -342,16 +342,20 @@ public abstract class HttpChannel { LOG.ignore(e); } - catch (ServletException e) - { - // TODO - } catch (EofException e) { LOG.debug(e); async_exception=e; _request.setHandled(true); } + catch (ServletException e) + { + LOG.warn(String.valueOf(_uri),e.toString()); + LOG.debug(String.valueOf(_uri),e); + async_exception=e; + _request.setHandled(true); + commitError(500, null, e.toString()); + } catch (Throwable e) { LOG.warn(String.valueOf(_uri),e); @@ -364,14 +368,12 @@ public abstract class HttpChannel handling = !_state.unhandle(); } } - } finally { __currentChannel.set(null); if (threadName!=null) Thread.currentThread().setName(threadName); - if (_state.isUncompleted()) { @@ -397,16 +399,14 @@ public abstract class HttpChannel } catch(IOException e) { - LOG.debug(e); + LOG.warn(e); } _request.setHandled(true); completed(); } - } } - /* ------------------------------------------------------------ */ protected boolean commitError(final int status, final String reason, String content) { @@ -583,7 +583,7 @@ public abstract class HttpChannel case CONTENT_TYPE: MimeTypes.Type mime=MimeTypes.CACHE.get(value); - String charset=(mime==null)?MimeTypes.getCharsetFromContentType(value):mime.getCharset().toString(); + String charset=(mime==null||mime.getCharset()==null)?MimeTypes.getCharsetFromContentType(value):mime.getCharset().toString(); if (charset!=null) _request.setCharacterEncodingUnchecked(charset); break; @@ -643,6 +643,10 @@ public abstract class HttpChannel @Override public boolean content(ByteBuffer ref) { + if (LOG.isDebugEnabled()) + { + LOG.debug("{} content {}",this,BufferUtil.toDetailString(ref)); + } _in.content(ref); return true; } 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 d17077764f9..81f3e9129e8 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 @@ -87,6 +87,7 @@ public class HttpConnection extends AbstractAsyncConnection _parser = new HttpParser(_channel.getEventHandler()); _generator = new HttpGenerator(); + _generator.setSendServerVersion(_server.getSendServerVersion()); LOG.debug("New HTTP Connection {}",this); } @@ -132,15 +133,21 @@ public class HttpConnection extends AbstractAsyncConnection _generator.reset(); _channel.reset(); _httpInput.recycle(); - if (_requestBuffer!=null) + if (_requestBuffer!=null && !_requestBuffer.hasRemaining()) + { _bufferPool.release(_requestBuffer); - _requestBuffer=null; - if (_responseHeader!=null) + _requestBuffer=null; + } + if (_responseHeader!=null && !_responseHeader.hasRemaining()) + { _bufferPool.release(_responseHeader); - _responseHeader=null; - if (_responseBuffer!=null) + _responseHeader=null; + } + if (_responseBuffer!=null && !_responseBuffer.hasRemaining()) + { _bufferPool.release(_responseBuffer); - _responseBuffer=null; + _responseBuffer=null; + } if (_chunk!=null) _bufferPool.release(_chunk); _chunk=null; @@ -252,11 +259,16 @@ public class HttpConnection extends AbstractAsyncConnection } // return if the connection has been changed - if (getEndPoint().getAsyncConnection()!=null) + if (getEndPoint().getAsyncConnection()!=this) { getEndPoint().getAsyncConnection().onOpen(); return; } + } + else if (BufferUtil.hasContent(_requestBuffer)) + { + LOG.warn("STATE MACHINE FAILURE??? {} {}",_parser,BufferUtil.toDetailString(_requestBuffer)); + BufferUtil.clear(_requestBuffer); } } } @@ -347,27 +359,53 @@ public class HttpConnection extends AbstractAsyncConnection generate(null,Action.COMPLETE); } + + @Override + protected boolean commitError(int status, String reason, String content) + { + if (!super.commitError(status,reason,content)) + { + // We could not send the error, so a sudden close of the connection will at least tell + // the client something is wrong + getEndPoint().close(); + return false; + } + return true; + } + @Override protected void completed() { - LOG.debug("{} completed",this); + LOG.debug(BufferUtil.toDetailString(_requestBuffer)); HttpConnection.this.reset(); - // if there is a pipelined request and onReadable has returned - if (_requestBuffer!=null && getCurrentConnection()==null) - execute(new Runnable() - { - @Override public void run() {onReadable();} - }); - else if (_parser.isIdle()) - scheduleOnReadable(); - else if (!getEndPoint().isOutputShutdown() && _parser.getState()==HttpParser.State.SEEKING_EOF) + // if the onReadable method is not executing + if (getCurrentConnection()==null) { - // TODO This is a catch all indicating some protocol handling failure - // Currently needed for requests saying they are HTTP/2.0. - // This should be removed once better error handling is in place - LOG.warn("Endpoint output not shutdown when seeking EOF"); - getEndPoint().close(); + // TODO is there a race here? + + if (_parser.isIdle()) + { + // it wants to eat more + if (_requestBuffer==null) + scheduleOnReadable(); + else + { + LOG.debug("{} pipelined",this); + execute(new Runnable() + { + @Override public void run() {onReadable();} + }); + } + } + else if (!getEndPoint().isOutputShutdown() && _parser.getState()==HttpParser.State.SEEKING_EOF) + { + // TODO This is a catch all indicating some protocol handling failure + // Currently needed for requests saying they are HTTP/2.0. + // This should be removed once better error handling is in place + LOG.warn("Endpoint output not shutdown when seeking EOF"); + getEndPoint().close(); + } } } @@ -421,8 +459,10 @@ public class HttpConnection extends AbstractAsyncConnection case FLUSH: if (_info.isHead()) { - BufferUtil.clear(_chunk); - BufferUtil.clear(_responseBuffer); + if (_chunk!=null) + BufferUtil.clear(_chunk); + if (_responseBuffer!=null) + BufferUtil.clear(_responseBuffer); } write(_responseHeader,_chunk,_responseBuffer).get(); continue; @@ -430,8 +470,10 @@ public class HttpConnection extends AbstractAsyncConnection case FLUSH_CONTENT: if (_info.isHead()) { - BufferUtil.clear(_chunk); - BufferUtil.clear(content); + if (_chunk!=null) + BufferUtil.clear(_chunk); + if (_responseBuffer!=null) + BufferUtil.clear(content); } write(_responseHeader,_chunk,content).get(); break; @@ -512,8 +554,10 @@ public class HttpConnection extends AbstractAsyncConnection case FLUSH: if (_info.isHead()) { - BufferUtil.clear(_chunk); - BufferUtil.clear(_responseBuffer); + if (_chunk!=null) + BufferUtil.clear(_chunk); + if (_responseBuffer!=null) + BufferUtil.clear(_responseBuffer); } write(_responseHeader,_chunk,_responseBuffer).get(); break; @@ -521,8 +565,10 @@ public class HttpConnection extends AbstractAsyncConnection case FLUSH_CONTENT: if (_info.isHead()) { - BufferUtil.clear(_chunk); - BufferUtil.clear(content); + if (_chunk!=null) + BufferUtil.clear(_chunk); + if (_responseBuffer!=null) + BufferUtil.clear(content); } // TODO need a proper call back to complete. write(_responseHeader,_chunk,content); @@ -614,11 +660,10 @@ public class HttpConnection extends AbstractAsyncConnection that uses the calling thread to block on a readable callback and then to do the parsing before before attempting the read. */ - - + // While progress and the connection has not changed - boolean parsed_event=false; - while (!getEndPoint().isInputShutdown()) + boolean parsed_event=_parser.parseNext(_requestBuffer==null?BufferUtil.EMPTY_BUFFER:_requestBuffer); + while (!parsed_event && !getEndPoint().isInputShutdown()) { try { diff --git a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java index ab3894465e6..bed174599aa 100644 --- a/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java +++ b/jetty-server/src/main/java/org/eclipse/jetty/server/HttpInput.java @@ -176,7 +176,7 @@ public abstract class HttpInput extends ServletInputStream public void shutdownInput() { synchronized (_inputQ.lock()) - { + { _inputEOF=true; } } diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java index 045f88d3be8..299c88e2502 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestBase.java @@ -40,6 +40,7 @@ import javax.servlet.http.HttpServletResponse; import junit.framework.Assert; import org.eclipse.jetty.io.EndPoint; +import org.eclipse.jetty.server.HttpServerTestFixture.EchoHandler; import org.eclipse.jetty.server.handler.AbstractHandler; import org.eclipse.jetty.util.IO; import org.eclipse.jetty.util.StringUtil; @@ -171,6 +172,36 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture } } + @Test + public void testTrailingContent() throws Exception + { + configureServer(new EchoHandler()); + + Socket client=newSocket(HOST,_connector.getLocalPort()); + try + { + OutputStream os=client.getOutputStream(); + + os.write(("GET /R2 HTTP/1.1\015\012"+ + "Host: localhost\015\012"+ + "Content-Length: 5\015\012"+ + "Content-Type: text/plain\015\012"+ + "Connection: close\015\012"+ + "\015\012"+ + "ABCDE\015\012"+ + "\015\012" + ).getBytes()); + os.flush(); + + // Read the response. + String response=readResponse(client); + assertTrue (response.indexOf("200")>0); + } + finally + { + client.close(); + } + } /* * Feed the server fragmentary headers and see how it copes with it. */ @@ -676,14 +707,12 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture LineNumberReader in = new LineNumberReader(new InputStreamReader(client.getInputStream())); String line = in.readLine(); - System.err.println(line); int count=0; while (line!=null) { if ("HTTP/1.1 200 OK".equals(line)) count++; line = in.readLine(); - System.err.println(line); } assertEquals(pipeline,count); } @@ -819,8 +848,9 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture ).getBytes("iso-8859-1")); + String in = IO.toString(is); - + int index=in.indexOf("123456789"); assertTrue(index>0); index=in.indexOf("123456789",index+1); @@ -944,7 +974,7 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture Socket client=newSocket(HOST,_connector.getLocalPort()); try { - ((StdErrLog)Log.getLogger(HttpConnection.class)).setHideStacks(true); + // ((StdErrLog)Log.getLogger(HttpConnection.class)).setHideStacks(true); OutputStream os=client.getOutputStream(); InputStream is=client.getInputStream(); @@ -956,6 +986,7 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture ).getBytes()); os.flush(); + client.setSoTimeout(2000); String in = IO.toString(is); @@ -972,7 +1003,7 @@ public abstract class HttpServerTestBase extends HttpServerTestFixture } finally { - ((StdErrLog)Log.getLogger(HttpConnection.class)).setHideStacks(false); + // ((StdErrLog)Log.getLogger(HttpConnection.class)).setHideStacks(false); if (!client.isClosed()) client.close(); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java index 3573b7c21b3..8291d759c58 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/HttpServerTestFixture.java @@ -77,6 +77,7 @@ public class HttpServerTestFixture musthavecontent=false; } + @Override public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException { baseRequest.setHandled(true); @@ -92,15 +93,16 @@ public class HttpServerTestFixture int count=0; BufferedReader reader=request.getReader(); + if (request.getContentLength()!=0) { - String line; - - while ((line=reader.readLine())!=null) + String line=reader.readLine(); + while (line!=null) { writer.print(line); writer.print("\n"); count+=line.length(); + line=reader.readLine(); } } @@ -111,7 +113,7 @@ public class HttpServerTestFixture writer.println("No content"); } - + // just to be difficult reader.close(); writer.close(); diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/SelectChannelServerTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/SelectChannelServerTest.java index 23d58d16871..3d9aed0fec7 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/SelectChannelServerTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/SelectChannelServerTest.java @@ -12,7 +12,14 @@ // ======================================================================== package org.eclipse.jetty.server; +import static org.junit.Assert.assertTrue; + +import java.io.OutputStream; +import java.net.Socket; + +import org.eclipse.jetty.server.HttpServerTestFixture.EchoHandler; import org.junit.BeforeClass; +import org.junit.Test; /** * HttpServer Tester. @@ -31,5 +38,12 @@ public class SelectChannelServerTest extends HttpServerTestBase super.testCommittedError(); } + @Override + public void testFragmentedChunk() throws Exception + { + super.testFragmentedChunk(); + } + + }