diff --git a/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java b/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java index 5b48f70a597..cf8e99697fc 100644 --- a/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java +++ b/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java @@ -542,8 +542,15 @@ public HttpServerResponse bodyEndHandler(Handler handler) { } private void doSendFile(String filename, long offset, long length, Handler> resultHandler) { - ObjectUtil.checkPositiveOrZero(offset, "offset"); - ObjectUtil.checkPositiveOrZero(length, "length"); + ContextInternal ctx = vertx.getOrCreateContext(); + if (offset < 0) { + ctx.runOnContext((v) -> resultHandler.handle(Future.failedFuture("offset : " + offset + " (expected: >= 0)"))); + return; + } + if (length < 0) { + ctx.runOnContext((v) -> resultHandler.handle(Future.failedFuture("length : " + length + " (expected: >= 0)"))); + return; + } synchronized (conn) { checkValid(); if (headWritten) { @@ -553,7 +560,6 @@ private void doSendFile(String filename, long offset, long length, Handler resultHandler.handle(Future.failedFuture(new FileNotFoundException()))); } else { log.error("File not found: " + filename); @@ -566,7 +572,6 @@ private void doSendFile(String filename, long offset, long length, Handler resultHandler.handle(Future.failedFuture(exception))); } else { @@ -598,7 +603,6 @@ private void doSendFile(String filename, long offset, long length, Handler resultHandler.handle(Future.failedFuture(e))); } else { log.error("Failed to send file", e); @@ -607,7 +611,6 @@ private void doSendFile(String filename, long offset, long length, Handler { // write an empty last content to let the http encoder know the response is complete if (future.isSuccess()) { diff --git a/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java b/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java index 5d3e23ac0a5..f01dfabf3ac 100644 --- a/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java +++ b/src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java @@ -603,8 +603,14 @@ public Future sendFile(String filename, long offset, long length) { @Override public HttpServerResponse sendFile(String filename, long offset, long length, Handler> resultHandler) { - ObjectUtil.checkPositiveOrZero(offset, "offset"); - ObjectUtil.checkPositiveOrZero(length, "length"); + if (offset < 0) { + resultHandler.handle(Future.failedFuture("offset : " + offset + " (expected: >= 0)")); + return this; + } + if (length < 0) { + resultHandler.handle(Future.failedFuture("length : " + length + " (expected: >= 0)")); + return this; + } synchronized (conn) { checkValid(); } @@ -624,14 +630,6 @@ public HttpServerResponse sendFile(String filename, long offset, long length, Ha AsyncFile file = ar.result(); long fileLength = file.getReadLength(); long contentLength = Math.min(length, fileLength); - - // fail early before status code/headers are written to the response - if (contentLength < 0) { - Exception exception = new IllegalArgumentException("offset : " + offset + " is larger than the requested file length : " + fileLength); - h.handle(Future.failedFuture(exception)); - return; - } - if (headers.get(HttpHeaderNames.CONTENT_LENGTH) == null) { putHeader(HttpHeaderNames.CONTENT_LENGTH, String.valueOf(contentLength)); } diff --git a/src/main/java/io/vertx/core/http/impl/HttpUtils.java b/src/main/java/io/vertx/core/http/impl/HttpUtils.java index 55ad04e3880..259f961c6ec 100644 --- a/src/main/java/io/vertx/core/http/impl/HttpUtils.java +++ b/src/main/java/io/vertx/core/http/impl/HttpUtils.java @@ -988,15 +988,20 @@ static void resolveFile(VertxInternal vertx, String filename, long offset, long //i.e is not a directory try(RandomAccessFile raf = new RandomAccessFile(file_, "r")) { FileSystem fs = vertx.fileSystem(); - fs.open(filename, new OpenOptions().setCreate(false).setWrite(false), ar -> { - if (ar.succeeded()) { - AsyncFile file = ar.result(); - long contentLength = Math.min(length, file_.length() - offset); - file.setReadPos(offset); - file.setReadLength(contentLength); - } - resultHandler.handle(ar); - }); + fs.open(filename, new OpenOptions().setCreate(false).setWrite(false)) + .transform(ar -> { + if (ar.succeeded()) { + AsyncFile file = ar.result(); + long contentLength = Math.min(length, file_.length() - offset); + if (contentLength < 0) { + file.close(); + return Future.failedFuture("offset : " + offset + " is larger than the requested file length : " + file_.length()); + } + file.setReadPos(offset); + file.setReadLength(contentLength); + } + return (Future) ar; + }).onComplete(resultHandler); } catch (IOException e) { resultHandler.handle(Future.failedFuture(e)); } diff --git a/src/test/java/io/vertx/core/http/HttpTest.java b/src/test/java/io/vertx/core/http/HttpTest.java index e2caf491b23..a6f9704ffd8 100644 --- a/src/test/java/io/vertx/core/http/HttpTest.java +++ b/src/test/java/io/vertx/core/http/HttpTest.java @@ -2187,92 +2187,45 @@ public void testSendZeroRangeFile() throws Exception { } @Test - public void testSendOffsetIsHigherThanFileLengthForFile() throws Exception { - File f = setupFile("twenty_three_bytes.txt", TestUtils.randomAlphaString(23)); - server.requestHandler(res -> { - try { - res.response().sendFile(f.getAbsolutePath(), 33, 10) - .onFailure(throwable -> { - try { - res.response().setStatusCode(500).end(); - } catch (IllegalStateException illegalStateException) { - // should not reach here, the response should not be sent if the offset is negative - fail(illegalStateException); - } - }); - } catch (Exception ex) { - // a route.route().failureHandler() would handle failures during - // handling of the request. Here we simulate that scenario, and during - // handling of failure we would like to set a status code for example. - try { - res.response().setStatusCode(505).end(); - fail("Should not reach here, failures should be handled by res.response().onFailure() above"); - } catch (IllegalStateException exceptionWhenTheResponseIsAlreadySent) { - // should not reach here, the response should not be sent if the offset is negative - fail(exceptionWhenTheResponseIsAlreadySent); - } - } - }).listen(testAddress, onSuccess(res -> { - client.request(requestOptions) - .compose(HttpClientRequest::send) - .onComplete(onSuccess(response -> { - assertEquals(500, response.statusCode()); - testComplete(); - })); - })); - await(); + public void testSendFileOffsetIsHigherThanFileLength() throws Exception { + testSendFileWithFailure( + (resp, f) -> resp.sendFile(f.getAbsolutePath(), 33, 10), + err -> assertEquals("offset : 33 is larger than the requested file length : 23", err.getMessage())); } @Test public void testSendFileWithNegativeLength() throws Exception { - File f = setupFile("twenty_three_bytes.txt", TestUtils.randomAlphaString(23)); - server.requestHandler(res -> { - try { - res.response().sendFile(f.getAbsolutePath(), 0, -100) - .onFailure(throwable -> { - // should not reach here, the response should not be sent if the offset is negative - fail("Should not reach here"); - }); - } catch (Exception ex) { - assertEquals(IllegalArgumentException.class, ex.getClass()); - assertEquals("length : -100 (expected: >= 0)", ex.getMessage()); - // handle failure in sendFile - res.response().setStatusCode(500).end(); - } + testSendFileWithFailure((resp, f) -> resp.sendFile(f.getAbsolutePath(), 0, -100), err -> { + assertEquals("length : -100 (expected: >= 0)", err.getMessage()); }); - startServer(testAddress); - client.request(requestOptions) - .compose(HttpClientRequest::send) - .onComplete(onSuccess(response -> { - assertEquals(500, response.statusCode()); - testComplete(); - })); - - await(); } @Test public void testSendFileWithNegativeOffset() throws Exception { + testSendFileWithFailure((resp, f) -> resp.sendFile(f.getAbsolutePath(), -100, 23), err -> { + assertEquals("offset : -100 (expected: >= 0)", err.getMessage()); + }); + } + + private void testSendFileWithFailure(BiFunction> sendFile, Consumer checker) throws Exception { + waitFor(2); File f = setupFile("twenty_three_bytes.txt", TestUtils.randomAlphaString(23)); server.requestHandler(res -> { - try { - res.response().sendFile(f.getAbsolutePath(), -100, 23) - .onFailure(throwable -> { - // should not reach here, the response should not be sent if the offset is negative - fail("Should not reach here"); - }); - } catch (Exception ex) { - assertEquals(IllegalArgumentException.class, ex.getClass()); - assertEquals("offset : -100 (expected: >= 0)", ex.getMessage()); - res.response().setStatusCode(500).end(); - } + // Expected + sendFile + .apply(res.response(), f) + .andThen(onFailure(checker::accept)) + .recover(v -> res.response().setStatusCode(500).end()) + .onComplete(onSuccess(v -> { + complete(); + })); }); startServer(testAddress); client.request(requestOptions) .compose(HttpClientRequest::send) .onComplete(onSuccess(response -> { assertEquals(500, response.statusCode()); - testComplete(); + complete(); })); await();