Ensure we close chunks before sending trailer (and NOT again after)

FitNesseRoot/FitNesse/ReleaseNotes/content.txt

@@ -1,4 +1,6 @@
 !2 ${FITNESSE_VERSION}
+ * Issues fixed:
+  * Fix search page ([[1207][https://github.com/unclebob/fitnesse/issues/1207]])
 
 !2 20190409
  * Issues fixed:

src/fitnesse/http/ChunkedResponse.java

@@ -12,6 +12,8 @@ public class ChunkedResponse extends Response implements Closeable {
   private int bytesSent = 0;
   private boolean dontChunk = false;
   private ChunkedDataProvider chunckedDataProvider;
+  private boolean chunksClosed = false;
+  private boolean trailerClosed = false;
 
   public ChunkedResponse(String format, ChunkedDataProvider chunckedDataProvider) {
     super(format);
@@ -49,6 +51,9 @@ public void add(byte[] bytes) throws IOException {
     if (dontChunk) {
       sender.send(bytes);
     } else {
+      if (chunksClosed) {
+        throw new IllegalStateException("Cannot add bytes after closing chunks");
+      }
       String sizeLine = asHex(bytes.length) + CRLF;
       ByteBuffer chunk = ByteBuffer.allocate(sizeLine.length() + bytes.length + 2);
       chunk.put(sizeLine.getBytes()).put(bytes).put(CRLF.getBytes());
@@ -59,27 +64,37 @@ public void add(byte[] bytes) throws IOException {
 
   public void addTrailingHeader(String key, String value) throws IOException {
     if (!dontChunk) {
-      String header = key + ": " + value + CRLF;
+      if (trailerClosed) {
+        throw new IllegalStateException("Cannot add headers after closing trailer");
+      }
+      closeChunks();
+      String header = appendHeader(new StringBuilder(), key, value).toString();
       sender.send(header.getBytes());
     }
   }
 
   public void closeChunks() throws IOException {
     if (!dontChunk) {
-      sender.send(("0" + CRLF).getBytes());
+      if (!chunksClosed) {
+        chunksClosed = true;
+        sender.send(("0" + CRLF).getBytes());
+      }
     }
   }
 
   public void closeTrailer() throws IOException {
     if (!dontChunk) {
-      sender.send(CRLF.getBytes());
+      if (!trailerClosed) {
+        trailerClosed = true;
+        sender.send(CRLF.getBytes());
+      }
     }
   }
 
   @Override
   public void close() throws IOException {
-    closeTrailer();
     closeChunks();
+    closeTrailer();
     sender.close();
   }
 

src/fitnesse/http/Response.java

@@ -112,11 +112,15 @@ public final String makeHttpHeaders() {
     if (hasContent()) {
       addContentHeaders();
     }
-    StringBuffer text = new StringBuffer();
+    StringBuilder text = new StringBuilder();
     if (!Format.TEXT.contentType.equals(contentType)) {
       text.append("HTTP/1.1 ").append(status).append(" ").append(
         getReasonPhrase()).append(CRLF);
-      makeHeaders(text);
+
+      for (Entry<String, String> entry: headers.entrySet()) {
+        appendHeader(text, entry.getKey(), entry.getValue());
+      }
+
       text.append(CRLF);
     }
     return text.toString();
@@ -172,10 +176,9 @@ public byte[] getEncodedBytes(String value) throws UnsupportedEncodingException
     return value.getBytes(FileUtil.CHARENCODING);
   }
 
-  void makeHeaders(StringBuffer text) {
-    for (Entry<String, String> entry: headers.entrySet()) {
-      text.append(entry.getKey()).append(": ").append(entry.getValue()).append(CRLF);
-    }
+  protected StringBuilder appendHeader(StringBuilder builder, String header, String value) {
+    builder.append(header).append(": ").append(value).append(CRLF);
+    return builder;
   }
 
   protected void addContentHeaders() {

src/fitnesse/responders/run/SuiteResponder.java

@@ -335,7 +335,6 @@ synchronized void setClosed() {
   void closeHtmlResponse(int exitCode) throws IOException {
     if (!isClosed()) {
       setClosed();
-      response.closeChunks();
       response.addTrailingHeader("Exit-Code", String.valueOf(exitCode));
       response.close();
     }

test/fitnesse/http/ChunkedResponseTest.java

@@ -21,7 +21,7 @@ public class ChunkedResponseTest implements ResponseSender {
   private ChunkedResponse response;
   private boolean closed = false;
 
-  public StringBuffer buffer;
+  public StringBuilder buffer;
 
   @Override
   public void send(byte[] bytes) {
@@ -39,7 +39,7 @@ public void close() {
 
   @Before
   public void setUp() throws Exception {
-    buffer = new StringBuffer();
+    buffer = new StringBuilder();
 
     response = new ChunkedResponse("html", new MockChunkedDataProvider());
     response.sendTo(this);
@@ -70,16 +70,22 @@ public void xmlHeaders() throws Exception {
 
   @Test
   public void testOneChunk() throws Exception {
-    buffer = new StringBuffer();
+    buffer = new StringBuilder();
     response.add("some more text");
 
     String text = buffer.toString();
     assertEquals("e\r\nsome more text\r\n", text);
+
+    assertFalse(closed);
+    buffer = new StringBuilder();
+    response.close();
+    assertEquals("0\r\n\r\n", buffer.toString());
+    assertTrue(closed);
   }
 
   @Test
   public void testTwoChunks() throws Exception {
-    buffer = new StringBuffer();
+    buffer = new StringBuilder();
     response.add("one");
     response.add("two");
 
@@ -90,7 +96,7 @@ public void testTwoChunks() throws Exception {
   @Test
   public void testSimpleClosing() throws Exception {
     assertFalse(closed);
-    buffer = new StringBuffer();
+    buffer = new StringBuilder();
     response.close();
     String text = buffer.toString();
     assertEquals("0\r\n\r\n", text);
@@ -100,16 +106,30 @@ public void testSimpleClosing() throws Exception {
   @Test
   public void testClosingInSteps() throws Exception {
     assertFalse(closed);
-    buffer = new StringBuffer();
+    buffer = new StringBuilder();
     response.closeChunks();
     assertEquals("0\r\n", buffer.toString());
     assertFalse(closed);
-    buffer = new StringBuffer();
+    buffer = new StringBuilder();
     response.closeTrailer();
     assertEquals("\r\n", buffer.toString());
     assertFalse(closed);
+    buffer = new StringBuilder();
+    response.close();
+    assertEquals("", buffer.toString());
+    assertTrue(closed);
+  }
+
+  @Test
+  public void testTrailerAndClose() throws Exception {
+    assertFalse(closed);
+    buffer = new StringBuilder();
+    response.add("a chunk");
+    response.addTrailingHeader("Some-Header", "someValue");
+    response.addTrailingHeader("Other-Header", "a value");
     response.close();
     assertTrue(closed);
+    assertEquals("7\r\na chunk\r\n0\r\nSome-Header: someValue\r\nOther-Header: a value\r\n\r\n", buffer.toString());
   }
 
   @Test
@@ -133,12 +153,14 @@ public void testNoNullPointerException() throws Exception {
   @Test
   public void testTrailingHeaders() throws Exception {
     response.closeChunks();
-    buffer = new StringBuffer();
+    buffer = new StringBuilder();
     response.addTrailingHeader("Some-Header", "someValue");
     assertEquals("Some-Header: someValue\r\n", buffer.toString());
+    buffer = new StringBuilder();
     response.closeTrailer();
     response.close();
     assertTrue(closed);
+    assertEquals("\r\n", buffer.toString());
   }
 
   @Test