Send chunked file fixes

- Extract HTTP send file tests in their own hiearchy
- Avoid closing chunked file writes when using directly descriptors
- Avoid premature connection close when sending file with TLS with a configured idle timeout

Author: vietj

vertx-core/src/main/java/io/vertx/core/net/impl/NetServerImpl.java

@@ -266,16 +266,16 @@ protected void initChannel(ChannelPipeline pipeline, boolean ssl) {
     if (options.getLogActivity()) {
       pipeline.addLast("logging", new LoggingHandler(options.getActivityLogDataFormat()));
     }
-    if (ssl || !options.isFileRegionEnabled() || !vertx.transport().supportFileRegion() || (options.getTrafficShapingOptions() != null && options.getTrafficShapingOptions().getOutboundGlobalBandwidth() > 0)) {
-      // only add ChunkedWriteHandler when SSL is enabled or FileRegion isn't supported or when outbound traffic shaping is enabled
-      pipeline.addLast("chunkedWriter", new ChunkedWriteHandler());       // For large file / sendfile support
-    }
     int idleTimeout = options.getIdleTimeout();
     int readIdleTimeout = options.getReadIdleTimeout();
     int writeIdleTimeout = options.getWriteIdleTimeout();
     if (idleTimeout > 0 || readIdleTimeout > 0 || writeIdleTimeout > 0) {
       pipeline.addLast("idle", new IdleStateHandler(readIdleTimeout, writeIdleTimeout, idleTimeout, options.getIdleTimeoutUnit()));
     }
+    if (ssl || !options.isFileRegionEnabled() || !vertx.transport().supportFileRegion() || (options.getTrafficShapingOptions() != null && options.getTrafficShapingOptions().getOutboundGlobalBandwidth() > 0)) {
+      // only add ChunkedWriteHandler when SSL is enabled or FileRegion isn't supported or when outbound traffic shaping is enabled
+      pipeline.addLast("chunkedWriter", new ChunkedWriteHandler());       // For large file / sendfile support
+    }
   }
 
   protected GlobalTrafficShapingHandler createTrafficShapingHandler() {

vertx-core/src/main/java/io/vertx/core/net/impl/VertxConnection.java

@@ -499,7 +499,12 @@ public ChannelFuture sendFile(FileChannel fc, long offset, long length) {
     if (!supportsFileRegion()) {
       // Cannot use zero-copy
       try {
-        writeToChannel(new ChunkedNioFile(fc, offset, length, 8192), writeFuture);
+        writeToChannel(new ChunkedNioFile(fc, offset, length, 8192) {
+          @Override
+          public void close() {
+            // The called is responsible for closing the file
+          }
+        }, writeFuture);
       } catch (IOException e) {
         return chctx.newFailedFuture(e);
       }

vertx-core/src/test/java/io/vertx/test/http/HttpTestBase.java

@@ -23,8 +23,13 @@
 import io.vertx.test.proxy.HttpProxy;
 import io.vertx.test.proxy.SocksProxy;
 import io.vertx.test.proxy.TestProxyBase;
+import org.junit.Rule;
+import org.junit.rules.TemporaryFolder;
 
+import java.io.BufferedWriter;
 import java.io.File;
+import java.io.FileOutputStream;
+import java.io.OutputStreamWriter;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.TimeUnit;
@@ -43,12 +48,16 @@ public class HttpTestBase extends VertxTestBase {
   public static final String DEFAULT_HTTPS_HOST_AND_PORT = DEFAULT_HTTPS_HOST + ":" + DEFAULT_HTTPS_PORT;;
   public static final String DEFAULT_TEST_URI = "some-uri";
 
+  @Rule
+  public final TemporaryFolder testFolder = TemporaryFolder.builder().assureDeletion().build();
+
   protected HttpServer server;
   protected HttpClientAgent client;
   protected TestProxyBase proxy;
   protected SocketAddress testAddress;
   protected RequestOptions requestOptions;
   private File tmp;
+  private File testDir;
 
   protected HttpServerOptions createBaseServerOptions() {
     return new HttpServerOptions().setPort(DEFAULT_HTTP_PORT).setHost(DEFAULT_HTTP_HOST);
@@ -70,6 +79,12 @@ public void setUp() throws Exception {
     client = vertx.createHttpClient(createBaseClientOptions());
   }
 
+  @Override
+  public void after() throws Exception {
+    testDir = null;
+    super.after();
+  }
+
   /**
    * Override to disable domain sockets testing.
    */
@@ -163,4 +178,22 @@ protected void startProxy(String username, ProxyType proxyType) throws Exception
     proxy.username(username);
     proxy.start(vertx);
   }
+
+  protected File testDir() throws Exception {
+    if (testDir == null) {
+      testDir = testFolder.newFolder();
+    }
+    return testDir;
+  }
+
+  protected File setupFile(String fileName, String content) throws Exception {
+    File file = new File(testDir(), fileName);
+    if (file.exists()) {
+      file.delete();
+    }
+    BufferedWriter out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(file), "UTF-8"));
+    out.write(content);
+    out.close();
+    return file;
+  }
 }

vertx-core/src/test/java/io/vertx/tests/http/Http1xTest.java

@@ -24,7 +24,6 @@
 import io.vertx.core.http.impl.headers.HeadersMultiMap;
 import io.vertx.core.impl.SysProps;
 import io.vertx.core.internal.ContextInternal;
-import io.vertx.core.impl.Utils;
 import io.vertx.core.internal.VertxInternal;
 import io.vertx.core.internal.http.HttpServerRequestInternal;
 import io.vertx.core.json.JsonArray;
@@ -41,7 +40,6 @@
 import org.junit.Ignore;
 import org.junit.Test;
 
-import java.io.File;
 import java.util.*;
 import java.util.concurrent.*;
 import java.util.concurrent.atomic.AtomicBoolean;
@@ -3787,29 +3785,6 @@ private void testPerXXXPooling(Function<Integer, Future<HttpClientRequest>> requ
     await();
   }
 
-  @Test
-  public void testSendFileFailsWhenClientClosesConnection() throws Exception {
-    // 10 megs
-    final File f = setupFile("file.pdf", TestUtils.randomUnicodeString(10 * 1024 * 1024));
-    server.requestHandler(req -> {
-      try {
-        req.response().sendFile(f.getAbsolutePath()).onComplete(onFailure(err -> {
-          // Broken pipe
-          testComplete();
-        }));
-      } catch (Exception e) {
-        // this was the bug reported with issues/issue-80
-        fail(e);
-      }
-    });
-    startServer(testAddress);
-    vertx.createNetClient().connect(testAddress).onComplete(onSuccess(socket -> {
-      socket.write("GET / HTTP/1.1\r\n\r\n");
-      socket.close();
-    }));
-    await();
-  }
-
   // Use a raw socket to check the body response is effectively empty (it could be an empty chunk)
   protected MultiMap checkEmptyHttpResponse(HttpMethod method, int sc, MultiMap reqHeaders) throws Exception {
     server.requestHandler(req -> {
@@ -4870,82 +4845,6 @@ public void start(Promise<Void> startFuture) {
     await();
   }
 
-  @Test
-  public void testHttpServerWithIdleTimeoutSendChunkedFile() throws Exception {
-    // Does not pass reliably in CI (timeout)
-    Assume.assumeTrue(!vertx.isNativeTransportEnabled() && !Utils.isWindows());
-    int expected = 32 * 1024 * 1024;
-    File file = TestUtils.tmpFile(".dat", expected);
-    // Estimate the delay to transfer a file with a 1ms pause in chunks
-    int delay = retrieveFileFromServer(file, createBaseServerOptions());
-    // Now test with timeout relative to this delay
-    int timeout = delay / 2;
-    delay = retrieveFileFromServer(file, createBaseServerOptions().setIdleTimeout(timeout).setIdleTimeoutUnit(TimeUnit.MILLISECONDS));
-    assertTrue(delay > timeout);
-  }
-
-  private int retrieveFileFromServer(File file, HttpServerOptions options) throws Exception {
-    server.close().await();
-    server = vertx
-      .createHttpServer(options)
-      .requestHandler(
-        req -> {
-          req.response().sendFile(file.getAbsolutePath());
-        });
-    startServer(testAddress);
-    long now = System.currentTimeMillis();
-    Integer len = getFile().await();
-    assertEquals((int)len, file.length());
-    return (int) (System.currentTimeMillis() - now);
-  }
-
-  private Future<Integer> getFile() {
-    int[] length = {0};
-    return client.request(requestOptions)
-      .compose(req -> req.send()
-        .compose(resp -> {
-          resp.handler(buff -> {
-            length[0] += buff.length();
-            resp.pause();
-            vertx.setTimer(1, id -> {
-              resp.resume();
-            });
-          });
-          resp.exceptionHandler(this::fail);
-          return resp.end();
-        }))
-      .map(v -> length[0]);
-  }
-
-  @Test
-  public void testSendFilePipelined() throws Exception {
-    int n = 2;
-    waitFor(n);
-    File sent = TestUtils.tmpFile(".dat", 16 * 1024);
-    server.requestHandler(
-      req -> {
-        req.response().sendFile(sent.getAbsolutePath());
-      });
-    startServer(testAddress);
-    client.close();
-    client = vertx.createHttpClient(createBaseClientOptions().setPipelining(true), new PoolOptions().setHttp1MaxSize(1));
-    for (int i = 0;i < n;i++) {
-      client.request(requestOptions)
-        .compose(req -> req.send().compose(HttpClientResponse::body))
-        .onComplete(onSuccess(body -> {
-          complete();
-        }));
-    }
-    await();
-  }
-
-  @Test
-  public void testSendFileWithConnectionCloseHeader() throws Exception {
-    String content = TestUtils.randomUnicodeString(1024 * 1024 * 2);
-    sendFile("test-send-file.html", content, false,
-      () -> client.request(requestOptions).map(req -> req.putHeader(HttpHeaders.CONNECTION, "close")));
-  }
-
   @Test
   public void testResponseEndHandlersConnectionClose() throws Exception {
     waitFor(2);