Skip to content

Commit

Permalink
Improve HttpServerResponse sendFile implementations that should close…
Browse files Browse the repository at this point in the history 
… the file when an early failure is returned, failures also should all be asynchronous and not synchronous so the error flow will predicably happen in the returned future instead of requiring a check in the returned future and a try/catch block around the sendFile method.
  • Loading branch information
@vietj
vietj committed Feb 20, 2024
1 parent b81aa3e commit 60bb2f6
Show file tree
Hide file tree
Showing 4 changed files with 53 additions and 94 deletions.
15 changes: 9 additions & 6 deletions src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -542,8 +542,15 @@ public HttpServerResponse bodyEndHandler(Handler<Void> handler) {
}

private void doSendFile(String filename, long offset, long length, Handler<AsyncResult<Void>> 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) {
Expand All @@ -553,7 +560,6 @@ private void doSendFile(String filename, long offset, long length, Handler<Async

if (!file.exists()) {
if (resultHandler != null) {
ContextInternal ctx = vertx.getOrCreateContext();
ctx.runOnContext((v) -> resultHandler.handle(Future.failedFuture(new FileNotFoundException())));
} else {
log.error("File not found: " + filename);
Expand All @@ -566,7 +572,6 @@ private void doSendFile(String filename, long offset, long length, Handler<Async
// fail early before status code/headers are written to the response
if (contentLength < 0) {
if (resultHandler != null) {
ContextInternal ctx = vertx.getOrCreateContext();
Exception exception = new IllegalArgumentException("offset : " + offset + " is larger than the requested file length : " + file.length());
ctx.runOnContext((v) -> resultHandler.handle(Future.failedFuture(exception)));
} else {
Expand Down Expand Up @@ -598,7 +603,6 @@ private void doSendFile(String filename, long offset, long length, Handler<Async
} catch (IOException ignore) {
}
if (resultHandler != null) {
ContextInternal ctx = vertx.getOrCreateContext();
ctx.runOnContext((v) -> resultHandler.handle(Future.failedFuture(e)));
} else {
log.error("Failed to send file", e);
Expand All @@ -607,7 +611,6 @@ private void doSendFile(String filename, long offset, long length, Handler<Async
}
written = true;

ContextInternal ctx = vertx.getOrCreateContext();
channelFuture.addListener(future -> {
// write an empty last content to let the http encoder know the response is complete
if (future.isSuccess()) {
Expand Down
18 changes: 8 additions & 10 deletions src/main/java/io/vertx/core/http/impl/Http2ServerResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -603,8 +603,14 @@ public Future<Void> sendFile(String filename, long offset, long length) {

@Override
public HttpServerResponse sendFile(String filename, long offset, long length, Handler<AsyncResult<Void>> 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();
}
Expand All @@ -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));
}
Expand Down
23 changes: 14 additions & 9 deletions src/main/java/io/vertx/core/http/impl/HttpUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
Expand Down
91 changes: 22 additions & 69 deletions src/test/java/io/vertx/core/http/HttpTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<HttpServerResponse, File, Future<Void>> sendFile, Consumer<Throwable> 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();
Expand Down

0 comments on commit 60bb2f6

Please sign in to comment.