diff --git a/source/file-server/src/main/java/nl/aerius/fileserver/local/LocalFileController.java b/source/file-server/src/main/java/nl/aerius/fileserver/local/LocalFileController.java index 5db840e..c93ba20 100644 --- a/source/file-server/src/main/java/nl/aerius/fileserver/local/LocalFileController.java +++ b/source/file-server/src/main/java/nl/aerius/fileserver/local/LocalFileController.java @@ -17,12 +17,17 @@ package nl.aerius.fileserver.local; import java.io.IOException; +import java.nio.channels.Channels; +import java.nio.channels.SeekableByteChannel; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Profile; -import org.springframework.core.io.FileUrlResource; +import org.springframework.core.io.InputStreamResource; import org.springframework.core.io.Resource; import org.springframework.http.ContentDisposition; import org.springframework.http.HttpHeaders; @@ -65,12 +70,17 @@ public ResponseEntity getFile(final @PathVariable String uuid, final @ final String file = storageService.getFile(uuid, filename); LOG.debug("Returning file: {}", file); - final FileUrlResource resource = new FileUrlResource(file); + // Open the file once and take both the length and the bytes from that one handle. A FileUrlResource reads the length + // for the Content-Length header first and opens the file later, so a concurrent overwrite in between makes Tomcat + // send a truncated (or never completed) body. An open handle keeps the old file even after it is renamed over. + final SeekableByteChannel channel = Files.newByteChannel(Path.of(file), StandardOpenOption.READ); + final long contentLength = channel.size(); + final InputStreamResource resource = new InputStreamResource(Channels.newInputStream(channel)); final HttpHeaders headers = new HttpHeaders(); final ContentDisposition contentDisposition = ContentDisposition.attachment().filename(filename).build(); headers.setContentDisposition(contentDisposition); - return ResponseEntity.ok().headers(headers).body(resource); + return ResponseEntity.ok().headers(headers).contentLength(contentLength).body(resource); } catch (final IOException e) { LOG.trace("IOException when trying to get a file", e); } catch (final RuntimeException e) { diff --git a/source/file-server/src/test/java/nl/aerius/fileserver/local/LocalFileControllerTest.java b/source/file-server/src/test/java/nl/aerius/fileserver/local/LocalFileControllerTest.java index a92be9f..c50b753 100644 --- a/source/file-server/src/test/java/nl/aerius/fileserver/local/LocalFileControllerTest.java +++ b/source/file-server/src/test/java/nl/aerius/fileserver/local/LocalFileControllerTest.java @@ -17,6 +17,7 @@ package nl.aerius.fileserver.local; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNotNull; @@ -32,7 +33,11 @@ import java.io.File; import java.io.FileNotFoundException; import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardCopyOption; +import java.util.List; import java.util.UUID; +import java.util.concurrent.atomic.AtomicBoolean; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.io.TempDir; @@ -120,6 +125,47 @@ void testGetFile() throws Exception { assertEquals(content, response.getContentAsString(), "Expects file content to be in data"); } + @Test + void testGetFileWhileOverwritten() throws Exception { + // A file that is atomically replaced while it is being served must still come back whole: the Content-Length header and + // the body have to describe the same version of the file. Reading the length first and opening the file later serves the + // new bytes under the old length, which the client then sees as a truncated body. + final String tempFilename = UUID.randomUUID().toString(); + final Path file = new File(tempDir, tempFilename).toPath(); + final String small = "{\"errors\":[],\"warnings\":[]}"; + final String large = "{\"errors\":[\"" + "x".repeat(4000) + "\"]}"; + Files.writeString(file, small); + doReturn(file.toAbsolutePath().toString()).when(storageService).getFile(UUID_CODE, tempFilename); + + final AtomicBoolean running = new AtomicBoolean(true); + final Thread writer = new Thread(() -> { + try { + while (running.get()) { + for (final String content : List.of(large, small)) { + final Path staged = Files.createTempFile(tempDir.toPath(), tempFilename, ".tmp"); + Files.writeString(staged, content); + Files.move(staged, file, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); + } + } + } catch (final Exception e) { + throw new IllegalStateException(e); + } + }); + writer.start(); + try { + for (int i = 0; i < 2000; i++) { + final MockHttpServletResponse response = mvc.perform(get(HTTP_LOCALHOST + UUID_CODE + "/" + tempFilename)).andExpect(status().isOk()) + .andReturn().getResponse(); + final String body = response.getContentAsString(); + assertEquals(body.length(), response.getContentLength(), "Content-Length must match the body that was served (iteration " + i + ")"); + assertTrue(body.equals(small) || body.equals(large), "Body must be one complete version of the file (iteration " + i + ")"); + } + } finally { + running.set(false); + writer.join(); + } + } + @Test void testGetFile404Missing() throws Exception { doThrow(new FileNotFoundException()).when(storageService).getFile(any(), any());