Skip to content

Commit f1b9d4f

Browse files
vietjvietj
committed
Avoid closing FileChannel/RandomAccessFile passed to HttpServerResponse#sendFile.
Motivation: FileChannel/RandomAccessFile passed to HttpServerResponse#sendFile are currently closed and they should not be per contract.
1 parent b9d8551 commit f1b9d4f

4 files changed

Lines changed: 117 additions & 93 deletions

File tree

‎vertx-core/src/main/java/io/vertx/core/http/impl/Http1xServerResponse.java‎

Lines changed: 79 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -442,10 +442,12 @@ public Future<Void> end() {
442442

443443
@Override
444444
public Future<Void> sendFile(String filename, long offset, long length) {
445-
RandomAccessFile raf;
446445
File file = vertx.fileResolver().resolve(filename);
446+
RandomAccessFile raf;
447+
long size;
447448
try {
448449
raf = new RandomAccessFile(file, "r");
450+
size = raf.length();
449451
} catch (Exception e) {
450452
return context.failedFuture(e);
451453
}
@@ -456,14 +458,7 @@ public Future<Void> sendFile(String filename, long offset, long length) {
456458
}
457459
headers.set(CONTENT_TYPE, mimeType);
458460
}
459-
Future<Void> result = sendFile(raf, offset, length);
460-
if (result.failed()) {
461-
try {
462-
raf.close();
463-
} catch (IOException ignored) {
464-
}
465-
}
466-
return result;
461+
return sendFileInternal(offset, length, size, raf, null, true);
467462
}
468463

469464
@Override
@@ -477,7 +472,7 @@ public Future<Void> sendFile(RandomAccessFile file, long offset, long length) {
477472
} catch (IOException e) {
478473
return context.failedFuture(e);
479474
}
480-
return sendFileInternal(offset, length, size, null, file);
475+
return sendFileInternal(offset, length, size, file, null, false);
481476
}
482477

483478
@Override
@@ -491,75 +486,91 @@ public Future<Void> sendFile(FileChannel channel, long offset, long length) {
491486
} catch (IOException e) {
492487
return context.failedFuture(e);
493488
}
494-
return sendFileInternal(offset, length, size, channel, null);
489+
return sendFileInternal(offset, length, size, null, channel, false);
495490
}
496491

497-
private <F> Future<Void> sendFileInternal(long offset, long length, long size, FileChannel fileChannel, RandomAccessFile raf) {
498-
ContextInternal ctx = vertx.getOrCreateContext();
499-
if (offset < 0) {
500-
return ctx.failedFuture("offset : " + offset + " (expected: >= 0)");
501-
}
502-
if (length < 0) {
503-
return ctx.failedFuture("length : " + length + " (expected: >= 0)");
504-
}
505-
synchronized (conn) {
506-
checkValid();
507-
if (headWritten) {
508-
throw new IllegalStateException("Head already written");
492+
private Future<Void> sendFileInternal(long offset, long length, long size, RandomAccessFile file, FileChannel fileChannel, boolean close) {
493+
Future<Void> ret = null;
494+
try {
495+
ContextInternal ctx = vertx.getOrCreateContext();
496+
if (offset < 0) {
497+
return ctx.failedFuture("offset : " + offset + " (expected: >= 0)");
498+
}
499+
if (length < 0) {
500+
return ctx.failedFuture("length : " + length + " (expected: >= 0)");
509501
}
510-
511502
long actualLength = Math.min(length, size - offset);
512503
long actualOffset = Math.min(offset, size);
513-
514-
// fail early before status code/headers are written to the response
515504
if (actualLength < 0) {
516505
return ctx.failedFuture("offset : " + offset + " is larger than the requested file length : " + size);
517506
}
518-
519-
prepareHeaders(actualLength);
520-
bytesWritten = actualLength;
521-
written = true;
522-
523-
conn.write(new VertxAssembledHttpResponse(head, version, status, headers), null);
524-
525-
ChannelFuture channelFut;
526-
if (fileChannel != null) {
527-
channelFut = conn.sendFile(fileChannel, actualOffset, actualLength);
528-
} else {
529-
channelFut = conn.sendFile(raf, actualOffset, actualLength);
530-
}
531-
channelFut.addListener(future -> {
532-
533-
// write an empty last content to let the http encoder know the response is complete
534-
if (future.isSuccess()) {
535-
conn.write(new VertxLastHttpContent(Unpooled.buffer(0), DefaultHttpHeadersFactory.trailersFactory().newHeaders()), null);
536-
}
537-
538-
// signal body end handler
539-
Handler<Void> handler;
540-
synchronized (conn) {
541-
handler = bodyEndHandler;
542-
}
543-
if (handler != null) {
544-
ctx.emit(handler);
507+
synchronized (conn) {
508+
checkValid();
509+
if (headWritten) {
510+
throw new IllegalStateException("Head already written");
545511
}
546512

547-
// allow to write next response
548-
// conn.responseComplete();
549-
550-
// signal end handler
551-
Handler<Void> end;
552-
synchronized (conn) {
553-
end = !closed ? endHandler : null;
554-
}
555-
if (null != end) {
556-
ctx.emit(end);
513+
// fail early before status code/headers are written to the response
514+
prepareHeaders(actualLength);
515+
bytesWritten = actualLength;
516+
written = true;
517+
conn.write(new VertxAssembledHttpResponse(head, version, status, headers), null);
518+
FileChannel toSend = fileChannel == null ? file.getChannel() : fileChannel;
519+
ChannelFuture channelFuture = conn.sendFile(toSend, actualOffset, actualLength);
520+
PromiseInternal<Void> promise = context.promise();
521+
ret = promise.future();
522+
channelFuture.addListener(future -> {
523+
if (future.isSuccess()) {
524+
525+
// signal body end handler
526+
Handler<Void> handler;
527+
synchronized (conn) {
528+
handler = bodyEndHandler;
529+
}
530+
if (handler != null) {
531+
ctx.emit(handler);
532+
}
533+
534+
// signal end handler
535+
Handler<Void> end;
536+
synchronized (conn) {
537+
end = !closed ? endHandler : null;
538+
}
539+
if (null != end) {
540+
ctx.emit(end);
541+
}
542+
543+
// write an empty last content to let the http encoder know the response is complete
544+
conn.write(new VertxLastHttpContent(Unpooled.buffer(0), DefaultHttpHeadersFactory.trailersFactory().newHeaders()), promise);
545+
} else {
546+
promise.fail(future.cause());
547+
}
548+
549+
//
550+
if (close) {
551+
try {
552+
if (file != null) {
553+
file.close();
554+
} else {
555+
fileChannel.close();
556+
}
557+
} catch (IOException ignore) {
558+
}
559+
}
560+
});
561+
}
562+
return ret;
563+
} finally {
564+
if (ret == null && close) {
565+
try {
566+
if (file != null) {
567+
file.close();
568+
} else {
569+
fileChannel.close();
570+
}
571+
} catch (IOException ignore) {
557572
}
558-
});
559-
560-
PromiseInternal<Void> promise = ctx.promise();
561-
channelFut.addListener(promise);
562-
return promise.future();
573+
}
563574
}
564575
}
565576

‎vertx-core/src/main/java/io/vertx/core/net/impl/NetSocketImpl.java‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,7 @@
4141
import io.vertx.core.streams.impl.InboundBuffer;
4242

4343
import java.io.File;
44+
import java.io.IOException;
4445
import java.io.RandomAccessFile;
4546
import java.nio.charset.Charset;
4647
import java.util.UUID;
@@ -289,9 +290,14 @@ public Future<Void> sendFile(String filename, long offset, long length) {
289290
}
290291
long actualLength = Math.min(length, file.length() - offset);
291292
long actualOffset = Math.min(offset, file.length());
292-
ChannelFuture fut = sendFile(raf, actualOffset, actualLength);
293+
ChannelFuture fut = sendFile(raf.getChannel(), actualOffset, actualLength);
293294
fut.addListener(promise);
294-
return promise.future();
295+
return promise.future().andThen(ar -> {
296+
try {
297+
raf.close();
298+
} catch (IOException ignore) {
299+
}
300+
});
295301
}
296302

297303
public NetSocketImpl exceptionHandler(Handler<Throwable> handler) {

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

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -493,12 +493,6 @@ private void sendFileRegion(FileChannel fc, long offset, long length, ChannelPro
493493
}
494494
}
495495

496-
public ChannelFuture sendFile(RandomAccessFile raf, long offset, long length) {
497-
ChannelFuture writeFuture = sendFile(raf.getChannel(), offset, length);
498-
writeFuture.addListener(fut -> raf.close());
499-
return writeFuture;
500-
}
501-
502496
public ChannelFuture sendFile(FileChannel fc, long offset, long length) {
503497
// Write the content.
504498
ChannelPromise writeFuture = chctx.newPromise();

‎vertx-core/src/test/java/io/vertx/tests/http/HttpTest.java‎

Lines changed: 30 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2307,7 +2307,9 @@ private void testSendFileWithFailure(BiFunction<HttpServerResponse, File, Future
23072307
public void testSendFileWithFileChannel() throws Exception {
23082308
int fileLength = 16 * 1024 * 1024;
23092309
BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender = (file, response) -> response.sendFile(file.getChannel());
2310-
testSendFileWithFileChannel(fileLength, sender, "application/octet-stream", fileLength);
2310+
try (RandomAccessFile raf = testSendFileWithFileChannel(fileLength, sender, "application/octet-stream", fileLength)) {
2311+
assertTrue(raf.getChannel().isOpen());
2312+
}
23112313
}
23122314

23132315
@Test
@@ -2316,7 +2318,9 @@ public void testSendFileWithFileChannelAndExtension() throws Exception {
23162318
BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender = (file, response) -> response
23172319
.putHeader(HttpHeaders.CONTENT_TYPE, "video/mp4")
23182320
.sendFile(file.getChannel());
2319-
testSendFileWithFileChannel(fileLength, sender, "video/mp4", fileLength);
2321+
try (RandomAccessFile raf = testSendFileWithFileChannel(fileLength, sender, "video/mp4", fileLength)) {
2322+
assertTrue(raf.getChannel().isOpen());
2323+
}
23202324
}
23212325

23222326
@Test
@@ -2327,14 +2331,18 @@ public void testSendFileWithFileChannelRange() throws Exception {
23272331
BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender = (file, response) -> response
23282332
.putHeader(HttpHeaders.CONTENT_TYPE, "video/mp4")
23292333
.sendFile(file.getChannel(), offset, expectedRange);
2330-
testSendFileWithFileChannel(fileLength, sender, "video/mp4", expectedRange);
2334+
try (RandomAccessFile raf = testSendFileWithFileChannel(fileLength, sender, "video/mp4", expectedRange)) {
2335+
assertTrue(raf.getChannel().isOpen());
2336+
}
23312337
}
23322338

23332339
@Test
23342340
public void testSendFileWithRandomAccessFile() throws Exception {
23352341
int fileLength = 16 * 1024 * 1024;
23362342
BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender = (file, response) -> response.sendFile(file);
2337-
testSendFileWithFileChannel(fileLength, sender, "application/octet-stream", fileLength);
2343+
try (RandomAccessFile raf = testSendFileWithFileChannel(fileLength, sender, "application/octet-stream", fileLength)) {
2344+
raf.getFilePointer();
2345+
}
23382346
}
23392347

23402348
@Test
@@ -2343,7 +2351,9 @@ public void testSendFileWithRandomAccessFileAndExtension() throws Exception {
23432351
BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender = (file, response) -> response
23442352
.putHeader(HttpHeaders.CONTENT_TYPE, "video/mp4")
23452353
.sendFile(file);
2346-
testSendFileWithFileChannel(fileLength, sender, "video/mp4", fileLength);
2354+
try (RandomAccessFile raf = testSendFileWithFileChannel(fileLength, sender, "video/mp4", fileLength)) {
2355+
raf.getFilePointer();
2356+
}
23472357
}
23482358

23492359
@Test
@@ -2354,22 +2364,25 @@ public void testSendFileWithRandomAccessFileRange() throws Exception {
23542364
BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender = (file, response) -> response
23552365
.putHeader(HttpHeaders.CONTENT_TYPE, "video/mp4")
23562366
.sendFile(file, offset, expectedRange);
2357-
testSendFileWithFileChannel(fileLength, sender, "video/mp4", expectedRange);
2367+
try (RandomAccessFile raf = testSendFileWithFileChannel(fileLength, sender, "video/mp4", expectedRange)) {
2368+
raf.getFilePointer();
2369+
}
23582370
}
23592371

2360-
private void testSendFileWithFileChannel(int flen, BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender, String expectedContentType, long expectedLength) throws Exception {
2372+
private RandomAccessFile testSendFileWithFileChannel(int flen, BiFunction<RandomAccessFile, HttpServerResponse, Future<?>> sender,
2373+
String expectedContentType, long expectedLength) throws Exception {
23612374
Assume.assumeTrue(this instanceof Http1xTest);
23622375
File file = TestUtils.tmpFile(".dat", flen);
2363-
try (RandomAccessFile raf = new RandomAccessFile(file, "r")) {
2364-
server.requestHandler(req -> sender.apply(raf, req.response()).onComplete(onSuccess(v -> testComplete())));
2365-
startServer(testAddress);
2366-
Buffer body = client.request(requestOptions)
2367-
.compose(req -> req.send()
2368-
.expecting(HttpResponseExpectation.contentType(expectedContentType))
2369-
.compose(HttpClientResponse::body)).await();
2370-
assertEquals(body.length(), expectedLength);
2371-
await();
2372-
}
2376+
RandomAccessFile raf = new RandomAccessFile(file, "r");
2377+
server.requestHandler(req -> sender.apply(raf, req.response()).onComplete(onSuccess(v -> testComplete())));
2378+
startServer(testAddress);
2379+
Buffer body = client.request(requestOptions)
2380+
.compose(req -> req.send()
2381+
.expecting(HttpResponseExpectation.contentType(expectedContentType))
2382+
.compose(HttpClientResponse::body)).await();
2383+
assertEquals(body.length(), expectedLength);
2384+
await();
2385+
return raf;
23732386
}
23742387

23752388
@Test

0 commit comments

Comments
 (0)