From 752d741d55466ccd648437202242bf44bc75d13f Mon Sep 17 00:00:00 2001 From: leventov Date: Mon, 12 Sep 2016 18:47:35 +0300 Subject: [PATCH 1/7] Add ResourceHandler, FileUtils.map() returning ResourceHandler and improve resource closing in FileSmoosher and SmooshedFileMapper --- .../java/com/metamx/common/FileUtils.java | 54 +++++++++++++++++++ .../com/metamx/common/ResourceHandler.java | 38 +++++++++++++ .../metamx/common/io/smoosh/FileSmoosher.java | 37 +++++-------- .../common/io/smoosh/SmooshedFileMapper.java | 15 +++++- 4 files changed, 117 insertions(+), 27 deletions(-) create mode 100644 src/main/java/com/metamx/common/ResourceHandler.java diff --git a/src/main/java/com/metamx/common/FileUtils.java b/src/main/java/com/metamx/common/FileUtils.java index f267ff9f..6115ea39 100644 --- a/src/main/java/com/metamx/common/FileUtils.java +++ b/src/main/java/com/metamx/common/FileUtils.java @@ -23,6 +23,10 @@ import com.google.common.io.Files; import java.io.File; +import java.io.FileNotFoundException; +import java.io.IOException; +import java.nio.MappedByteBuffer; +import java.nio.channels.FileChannel; import java.util.Arrays; import java.util.Collection; @@ -125,4 +129,54 @@ public void addFile(File file) this.addFiles(ImmutableList.of(file)); } } + + /** + * Fully maps a file read-only in to memory as per + * {@link FileChannel#map(java.nio.channels.FileChannel.MapMode, long, long)}. + * + *

Files are mapped from offset 0 to its length. + * + *

This only works for files <= {@link Integer#MAX_VALUE} bytes. + * + *

Similar to {@link Files#map(File)}, but returns {@link ResourceHandler}, that makes it easier to unmap the + * buffer within try-with-resources pattern: + * 

{@code
+   * try (ResourceHandler fileMappingHandler = FileUtils.map(file)) {
+   *   ByteBuffer fileMapping = fileMappingHandler.get();
+   *   // use mapped buffer
+   * }}
+ * + * @param file the file to map + * @return a {@link ResourceHandler}, wrapping a read-only buffer reflecting {@code file} + * @throws FileNotFoundException if the {@code file} does not exist + * @throws IOException if an I/O error occurs + * + * @see FileChannel#map(FileChannel.MapMode, long, long) + */ + public static ResourceHandler map(File file) throws IOException + { + MappedByteBuffer mappedByteBuffer = Files.map(file); + return new MappedByteBufferHandler(mappedByteBuffer); + } + + private static class MappedByteBufferHandler implements ResourceHandler { + private final MappedByteBuffer mappedByteBuffer; + + private MappedByteBufferHandler(MappedByteBuffer mappedByteBuffer) + { + this.mappedByteBuffer = mappedByteBuffer; + } + + @Override + public MappedByteBuffer get() + { + return mappedByteBuffer; + } + + @Override + public void close() + { + ByteBufferUtils.unmap(mappedByteBuffer); + } + } } diff --git a/src/main/java/com/metamx/common/ResourceHandler.java b/src/main/java/com/metamx/common/ResourceHandler.java new file mode 100644 index 00000000..53aa3710 --- /dev/null +++ b/src/main/java/com/metamx/common/ResourceHandler.java @@ -0,0 +1,38 @@ +/* + * Copyright 2016 Metamarkets Group Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.metamx.common; + +/** + * Facilitates using try-with-resources with resources that don't implement {@link AutoCloseable}, e. g. + * {@link java.nio.ByteBuffer}s. + * + * @param the type of wrapped resource + */ +public interface ResourceHandler extends AutoCloseable +{ + /** + * Returns the wrapped resource. + */ + T get(); + + /** + * Closes the wrapped resource. + */ + @Override + void close(); +} diff --git a/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java b/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java index d741bbb3..ea5b867a 100644 --- a/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java +++ b/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java @@ -22,11 +22,11 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.io.ByteStreams; -import com.google.common.io.Closeables; -import com.google.common.io.Files; import com.google.common.primitives.Ints; +import com.metamx.common.FileUtils; import com.metamx.common.IAE; import com.metamx.common.ISE; +import com.metamx.common.ResourceHandler; import java.io.BufferedOutputStream; import java.io.BufferedWriter; @@ -40,6 +40,7 @@ import java.io.OutputStreamWriter; import java.io.Writer; import java.nio.ByteBuffer; +import java.nio.MappedByteBuffer; import java.nio.channels.Channels; import java.nio.channels.WritableByteChannel; import java.util.Arrays; @@ -85,19 +86,6 @@ public FileSmoosher( Preconditions.checkArgument(maxChunkSize > 0, "maxChunkSize must be a positive value."); } - private FileSmoosher( - File baseDir, - int maxChunkSize, - List outFiles, - Map internalFiles - ) - { - this.baseDir = baseDir; - this.maxChunkSize = maxChunkSize; - this.outFiles.addAll(outFiles); - this.internalFiles.putAll(internalFiles); - } - public Set getInternalFilenames() { return internalFiles.keySet(); @@ -105,12 +93,14 @@ public Set getInternalFilenames() public void add(File fileToAdd) throws IOException { - add(fileToAdd.getName(), Files.map(fileToAdd)); + add(fileToAdd.getName(), fileToAdd); } public void add(String name, File fileToAdd) throws IOException { - add(name, Files.map(fileToAdd)); + try (ResourceHandler fileMappingHandler = FileUtils.map(fileToAdd)) { + add(name, fileMappingHandler.get()); + } } public void add(String name, ByteBuffer bufferToAdd) throws IOException @@ -149,7 +139,7 @@ public SmooshedWriter addWithSmooshedWriter(final String name, final long size) currOut = getNewCurrOut(); } if (currOut.bytesLeft() < size) { - Closeables.close(currOut, false); + currOut.close(); currOut = getNewCurrOut(); } @@ -213,13 +203,13 @@ public void close() throws IOException @Override public void close() throws IOException { - Closeables.close(currOut, false); + if (currOut != null) { + currOut.close(); + } File metaFile = metaFile(baseDir); - Writer out = null; - try { - out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(metaFile), Charsets.UTF_8)); + try (Writer out = new BufferedWriter(new OutputStreamWriter(new FileOutputStream(metaFile), Charsets.UTF_8))) { out.write(String.format("v1,%d,%d", maxChunkSize, outFiles.size())); out.write("\n"); @@ -236,9 +226,6 @@ public void close() throws IOException out.write("\n"); } } - finally { - Closeables.close(out, false); - } } private Outer getNewCurrOut() throws FileNotFoundException diff --git a/src/main/java/com/metamx/common/io/smoosh/SmooshedFileMapper.java b/src/main/java/com/metamx/common/io/smoosh/SmooshedFileMapper.java index 23292af9..ecba8517 100644 --- a/src/main/java/com/metamx/common/io/smoosh/SmooshedFileMapper.java +++ b/src/main/java/com/metamx/common/io/smoosh/SmooshedFileMapper.java @@ -17,6 +17,7 @@ package com.metamx.common.io.smoosh; import com.google.common.base.Charsets; +import com.google.common.base.Throwables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.io.Closeables; @@ -131,10 +132,20 @@ public ByteBuffer mapFile(String name) throws IOException } @Override - public void close() throws IOException + public void close() { + Throwable thrown = null; for (MappedByteBuffer mappedByteBuffer : buffersList) { - ByteBufferUtils.unmap(mappedByteBuffer); + try { + ByteBufferUtils.unmap(mappedByteBuffer); + } catch (Throwable t) { + if (thrown == null) { + thrown = t; + } else { + thrown.addSuppressed(t); + } + } } + Throwables.propagateIfPossible(thrown); } } From f6b2d4a6395c389ab1910da6d4a08cb1f5d1a2ac Mon Sep 17 00:00:00 2001 From: leventov Date: Mon, 12 Sep 2016 21:20:21 +0300 Subject: [PATCH 2/7] Add replacement note to ResourceHandler --- src/main/java/com/metamx/common/ResourceHandler.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/main/java/com/metamx/common/ResourceHandler.java b/src/main/java/com/metamx/common/ResourceHandler.java index 53aa3710..669699b0 100644 --- a/src/main/java/com/metamx/common/ResourceHandler.java +++ b/src/main/java/com/metamx/common/ResourceHandler.java @@ -21,6 +21,8 @@ * Facilitates using try-with-resources with resources that don't implement {@link AutoCloseable}, e. g. * {@link java.nio.ByteBuffer}s. * + *

This interface replaces {@code io.druid.collections.ResourceHandler}. + * * @param the type of wrapped resource */ public interface ResourceHandler extends AutoCloseable From f1315de536c4305aac263a3571a2783376ba1a28 Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 14 Sep 2016 19:18:10 +0300 Subject: [PATCH 3/7] Specialize ResourceHandler as MappedByteBufferHandler --- .../java/com/metamx/common/FileUtils.java | 31 ++--------- .../common/MappedByteBufferHandler.java | 54 +++++++++++++++++++ .../com/metamx/common/ResourceHandler.java | 40 -------------- .../metamx/common/io/smoosh/FileSmoosher.java | 5 +- 4 files changed, 61 insertions(+), 69 deletions(-) create mode 100644 src/main/java/com/metamx/common/MappedByteBufferHandler.java delete mode 100644 src/main/java/com/metamx/common/ResourceHandler.java diff --git a/src/main/java/com/metamx/common/FileUtils.java b/src/main/java/com/metamx/common/FileUtils.java index 6115ea39..d4b29611 100644 --- a/src/main/java/com/metamx/common/FileUtils.java +++ b/src/main/java/com/metamx/common/FileUtils.java @@ -138,45 +138,24 @@ public void addFile(File file) * *

This only works for files <= {@link Integer#MAX_VALUE} bytes. * - *

Similar to {@link Files#map(File)}, but returns {@link ResourceHandler}, that makes it easier to unmap the - * buffer within try-with-resources pattern: + *

Similar to {@link Files#map(File)}, but returns {@link MappedByteBufferHandler}, that makes it easier to unmap + * the buffer within try-with-resources pattern: * 

{@code
-   * try (ResourceHandler fileMappingHandler = FileUtils.map(file)) {
+   * try (MappedByteBufferHandler fileMappingHandler = FileUtils.map(file)) {
    *   ByteBuffer fileMapping = fileMappingHandler.get();
    *   // use mapped buffer
    * }}
* * @param file the file to map - * @return a {@link ResourceHandler}, wrapping a read-only buffer reflecting {@code file} + * @return a {@link MappedByteBufferHandler}, wrapping a read-only buffer reflecting {@code file} * @throws FileNotFoundException if the {@code file} does not exist * @throws IOException if an I/O error occurs * * @see FileChannel#map(FileChannel.MapMode, long, long) */ - public static ResourceHandler map(File file) throws IOException + public static MappedByteBufferHandler map(File file) throws IOException { MappedByteBuffer mappedByteBuffer = Files.map(file); return new MappedByteBufferHandler(mappedByteBuffer); } - - private static class MappedByteBufferHandler implements ResourceHandler { - private final MappedByteBuffer mappedByteBuffer; - - private MappedByteBufferHandler(MappedByteBuffer mappedByteBuffer) - { - this.mappedByteBuffer = mappedByteBuffer; - } - - @Override - public MappedByteBuffer get() - { - return mappedByteBuffer; - } - - @Override - public void close() - { - ByteBufferUtils.unmap(mappedByteBuffer); - } - } } diff --git a/src/main/java/com/metamx/common/MappedByteBufferHandler.java b/src/main/java/com/metamx/common/MappedByteBufferHandler.java new file mode 100644 index 00000000..32f54dcb --- /dev/null +++ b/src/main/java/com/metamx/common/MappedByteBufferHandler.java @@ -0,0 +1,54 @@ +/* + * Copyright 2016 Metamarkets Group Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.metamx.common; + +import java.io.File; +import java.nio.MappedByteBuffer; + +/** + * Facilitates using try-with-resources with {@link MappedByteBuffer}s which don't implement {@link AutoCloseable}. + * + *

This interface is a specialization of {@code io.druid.collections.ResourceHandler}. + * @see FileUtils#map(File) + */ +public final class MappedByteBufferHandler implements AutoCloseable +{ + private final MappedByteBuffer mappedByteBuffer; + + MappedByteBufferHandler(MappedByteBuffer mappedByteBuffer) + { + this.mappedByteBuffer = mappedByteBuffer; + } + + /** + * Returns the wrapped buffer. + */ + public MappedByteBuffer get() + { + return mappedByteBuffer; + } + + /** + * Unmaps the wrapped buffer. + */ + @Override + public void close() + { + ByteBufferUtils.unmap(mappedByteBuffer); + } +} diff --git a/src/main/java/com/metamx/common/ResourceHandler.java b/src/main/java/com/metamx/common/ResourceHandler.java deleted file mode 100644 index 669699b0..00000000 --- a/src/main/java/com/metamx/common/ResourceHandler.java +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Copyright 2016 Metamarkets Group Inc. - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - */ - -package com.metamx.common; - -/** - * Facilitates using try-with-resources with resources that don't implement {@link AutoCloseable}, e. g. - * {@link java.nio.ByteBuffer}s. - * - *

This interface replaces {@code io.druid.collections.ResourceHandler}. - * - * @param the type of wrapped resource - */ -public interface ResourceHandler extends AutoCloseable -{ - /** - * Returns the wrapped resource. - */ - T get(); - - /** - * Closes the wrapped resource. - */ - @Override - void close(); -} diff --git a/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java b/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java index ea5b867a..68d89229 100644 --- a/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java +++ b/src/main/java/com/metamx/common/io/smoosh/FileSmoosher.java @@ -26,7 +26,7 @@ import com.metamx.common.FileUtils; import com.metamx.common.IAE; import com.metamx.common.ISE; -import com.metamx.common.ResourceHandler; +import com.metamx.common.MappedByteBufferHandler; import java.io.BufferedOutputStream; import java.io.BufferedWriter; @@ -40,7 +40,6 @@ import java.io.OutputStreamWriter; import java.io.Writer; import java.nio.ByteBuffer; -import java.nio.MappedByteBuffer; import java.nio.channels.Channels; import java.nio.channels.WritableByteChannel; import java.util.Arrays; @@ -98,7 +97,7 @@ public void add(File fileToAdd) throws IOException public void add(String name, File fileToAdd) throws IOException { - try (ResourceHandler fileMappingHandler = FileUtils.map(fileToAdd)) { + try (MappedByteBufferHandler fileMappingHandler = FileUtils.map(fileToAdd)) { add(name, fileMappingHandler.get()); } } From 9f18dac4b55b2fa9142d383758a5c1d9f01c4a6f Mon Sep 17 00:00:00 2001 From: leventov Date: Tue, 20 Sep 2016 20:55:57 +0300 Subject: [PATCH 4/7] Add SmooshedFileMapperTest.testDeterministicFileUnmapping() --- .../io/smoosh/SmooshedFileMapperTest.java | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java index e01cfa59..3eb0bbe5 100644 --- a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java +++ b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java @@ -24,8 +24,13 @@ import org.junit.Test; import java.io.File; +import java.io.IOException; +import java.io.RandomAccessFile; +import java.lang.management.BufferPoolMXBean; +import java.lang.management.ManagementFactory; import java.nio.ByteBuffer; import java.util.Arrays; +import java.util.List; /** */ @@ -35,11 +40,13 @@ public class SmooshedFileMapperTest public void testSanity() throws Exception { File baseDir = Files.createTempDir(); + baseDir.deleteOnExit(); try { FileSmoosher smoosher = new FileSmoosher(baseDir, 21); for (int i = 0; i < 20; ++i) { File tmpFile = File.createTempFile(String.format("smoosh-%s", i), ".bin"); + tmpFile.deleteOnExit(); Files.write(Ints.toByteArray(i), tmpFile); smoosher.add(String.format("%d", i), tmpFile); tmpFile.delete(); @@ -76,6 +83,7 @@ public void testSanity() throws Exception public void testBehaviorWhenReportedSizesLargeAndExceptionIgnored() throws Exception { File baseDir = Files.createTempDir(); + baseDir.deleteOnExit(); try { FileSmoosher smoosher = new FileSmoosher(baseDir, 21); @@ -116,6 +124,7 @@ public void testBehaviorWhenReportedSizesLargeAndExceptionIgnored() throws Excep public void testBehaviorWhenReportedSizesSmall() throws Exception { File baseDir = Files.createTempDir(); + baseDir.deleteOnExit(); try { FileSmoosher smoosher = new FileSmoosher(baseDir, 21); @@ -140,4 +149,49 @@ public void testBehaviorWhenReportedSizesSmall() throws Exception } } } + + @Test + public void testDeterministicFileUnmapping() throws IOException + { + File baseDir = Files.createTempDir(); + baseDir.deleteOnExit(); + + int fileSize = 1 << 20; // 1 MB + try { + long totalMemoryUsedBeforeAddingFile = totalMemoryUsedByDirectAndMappedBuffers(); + FileSmoosher smoosher = new FileSmoosher(baseDir); + File dataFile = createTempFileOfSize("data", "bin", fileSize); + smoosher.add(dataFile); + // In case smoosher maps some own files internally (though currently it is not), let it unmap them + smoosher.close(); + long totalMemoryUsedAfterAddingFile = totalMemoryUsedByDirectAndMappedBuffers(); + // Assert no hanging file mappings left by either smoosher or smoosher.add(file) + Assert.assertEquals(totalMemoryUsedBeforeAddingFile, totalMemoryUsedAfterAddingFile); + } + finally { + for (File file : baseDir.listFiles()) { + file.delete(); + } + } + } + + private static File createTempFileOfSize(String prefix, String suffix, int fileSize) throws IOException + { + File dataFile = File.createTempFile(prefix, suffix); + dataFile.deleteOnExit(); + try (RandomAccessFile raf = new RandomAccessFile(dataFile, "rw")) { + raf.setLength(fileSize); + } + return dataFile; + } + + private static long totalMemoryUsedByDirectAndMappedBuffers() + { + long totalMemoryUsed = 0L; + List pools = ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class); + for (BufferPoolMXBean pool : pools) { + totalMemoryUsed += pool.getMemoryUsed(); + } + return totalMemoryUsed; + } } From c9b246120245d9554add3ee4a9a622a817b3c9c3 Mon Sep 17 00:00:00 2001 From: leventov Date: Tue, 20 Sep 2016 22:25:23 +0300 Subject: [PATCH 5/7] Make SmooshedFileMapperTest to use JUnit's TemporaryFolder and try-with-resources; Add FileUtilsTest --- .../java/com/metamx/common/BufferUtils.java | 38 ++++ .../java/com/metamx/common/FileUtilsTest.java | 49 ++++++ .../io/smoosh/SmooshedFileMapperTest.java | 163 +++++++----------- 3 files changed, 146 insertions(+), 104 deletions(-) create mode 100644 src/test/java/com/metamx/common/BufferUtils.java create mode 100644 src/test/java/com/metamx/common/FileUtilsTest.java diff --git a/src/test/java/com/metamx/common/BufferUtils.java b/src/test/java/com/metamx/common/BufferUtils.java new file mode 100644 index 00000000..28f05083 --- /dev/null +++ b/src/test/java/com/metamx/common/BufferUtils.java @@ -0,0 +1,38 @@ +/* + * Copyright 2016 Metamarkets Group Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.metamx.common; + +import java.lang.management.BufferPoolMXBean; +import java.lang.management.ManagementFactory; +import java.util.List; + +public final class BufferUtils +{ + + public static long totalMemoryUsedByDirectAndMappedBuffers() + { + long totalMemoryUsed = 0L; + List pools = ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class); + for (BufferPoolMXBean pool : pools) { + totalMemoryUsed += pool.getMemoryUsed(); + } + return totalMemoryUsed; + } + + private BufferUtils() {} +} diff --git a/src/test/java/com/metamx/common/FileUtilsTest.java b/src/test/java/com/metamx/common/FileUtilsTest.java new file mode 100644 index 00000000..b3c32604 --- /dev/null +++ b/src/test/java/com/metamx/common/FileUtilsTest.java @@ -0,0 +1,49 @@ +/* + * Copyright 2016 Metamarkets Group Inc. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package com.metamx.common; + +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; + +import java.io.File; +import java.io.IOException; +import java.io.RandomAccessFile; + +public class FileUtilsTest +{ + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + + @Test + public void testMap() throws IOException + { + File dataFile = folder.newFile("data"); + try (RandomAccessFile raf = new RandomAccessFile(dataFile, "rw")) { + raf.write(42); + raf.setLength(1 << 20); // 1 MB + } + long buffersMemoryBefore = BufferUtils.totalMemoryUsedByDirectAndMappedBuffers(); + try (MappedByteBufferHandler mappedByteBufferHandler = FileUtils.map(dataFile)) { + Assert.assertEquals(42, mappedByteBufferHandler.get().get(0)); + } + long buffersMemoryAfter = BufferUtils.totalMemoryUsedByDirectAndMappedBuffers(); + Assert.assertEquals(buffersMemoryBefore, buffersMemoryAfter); + } +} diff --git a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java index 3eb0bbe5..129b17df 100644 --- a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java +++ b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java @@ -18,51 +18,49 @@ import com.google.common.io.Files; import com.google.common.primitives.Ints; +import com.metamx.common.BufferUtils; import com.metamx.common.ISE; -import com.metamx.common.guava.CloseQuietly; import junit.framework.Assert; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; import java.io.File; import java.io.IOException; import java.io.RandomAccessFile; -import java.lang.management.BufferPoolMXBean; -import java.lang.management.ManagementFactory; import java.nio.ByteBuffer; import java.util.Arrays; -import java.util.List; /** */ public class SmooshedFileMapperTest { + @Rule + public TemporaryFolder folder = new TemporaryFolder(); + @Test public void testSanity() throws Exception { - File baseDir = Files.createTempDir(); - baseDir.deleteOnExit(); + File baseDir = folder.newFolder("base"); - try { - FileSmoosher smoosher = new FileSmoosher(baseDir, 21); + try (FileSmoosher smoosher = new FileSmoosher(baseDir, 21)) { for (int i = 0; i < 20; ++i) { - File tmpFile = File.createTempFile(String.format("smoosh-%s", i), ".bin"); - tmpFile.deleteOnExit(); + File tmpFile = folder.newFile(String.format("smoosh-%s.bin", i)); Files.write(Ints.toByteArray(i), tmpFile); smoosher.add(String.format("%d", i), tmpFile); - tmpFile.delete(); } - smoosher.close(); + } - File[] files = baseDir.listFiles(); - Arrays.sort(files); + File[] files = baseDir.listFiles(); + Arrays.sort(files); - Assert.assertEquals(5, files.length); // 4 smooshed files and 1 meta file - for (int i = 0; i < 4; ++i) { - Assert.assertEquals(FileSmoosher.makeChunkFile(baseDir, i), files[i]); - } - Assert.assertEquals(FileSmoosher.metaFile(baseDir), files[files.length - 1]); + Assert.assertEquals(5, files.length); // 4 smooshed files and 1 meta file + for (int i = 0; i < 4; ++i) { + Assert.assertEquals(FileSmoosher.makeChunkFile(baseDir, i), files[i]); + } + Assert.assertEquals(FileSmoosher.metaFile(baseDir), files[files.length - 1]); - SmooshedFileMapper mapper = SmooshedFileMapper.load(baseDir); + try (SmooshedFileMapper mapper = SmooshedFileMapper.load(baseDir)) { for (int i = 0; i < 20; ++i) { ByteBuffer buf = mapper.mapFile(String.format("%d", i)); Assert.assertEquals(0, buf.position()); @@ -70,40 +68,38 @@ public void testSanity() throws Exception Assert.assertEquals(4, buf.capacity()); Assert.assertEquals(i, buf.getInt()); } - mapper.close(); - } - finally { - for (File file : baseDir.listFiles()) { - file.delete(); - } } } @Test public void testBehaviorWhenReportedSizesLargeAndExceptionIgnored() throws Exception { - File baseDir = Files.createTempDir(); - baseDir.deleteOnExit(); + File baseDir = folder.newFolder("base"); - try { - FileSmoosher smoosher = new FileSmoosher(baseDir, 21); + try (FileSmoosher smoosher = new FileSmoosher(baseDir, 21)) { for (int i = 0; i < 20; ++i) { final SmooshedWriter writer = smoosher.addWithSmooshedWriter(String.format("%d", i), 7); writer.write(ByteBuffer.wrap(Ints.toByteArray(i))); - CloseQuietly.close(writer); + try { + writer.close(); + Assert.fail("IOException expected"); + } + catch (IOException ignored) { + // expected + } } - smoosher.close(); + } - File[] files = baseDir.listFiles(); - Arrays.sort(files); + File[] files = baseDir.listFiles(); + Arrays.sort(files); - Assert.assertEquals(6, files.length); // 4 smoosh files and 1 meta file - for (int i = 0; i < 4; ++i) { - Assert.assertEquals(FileSmoosher.makeChunkFile(baseDir, i), files[i]); - } - Assert.assertEquals(FileSmoosher.metaFile(baseDir), files[files.length - 1]); + Assert.assertEquals(6, files.length); // 4 smoosh files and 1 meta file + for (int i = 0; i < 4; ++i) { + Assert.assertEquals(FileSmoosher.makeChunkFile(baseDir, i), files[i]); + } + Assert.assertEquals(FileSmoosher.metaFile(baseDir), files[files.length - 1]); - SmooshedFileMapper mapper = SmooshedFileMapper.load(baseDir); + try (SmooshedFileMapper mapper = SmooshedFileMapper.load(baseDir)) { for (int i = 0; i < 20; ++i) { ByteBuffer buf = mapper.mapFile(String.format("%d", i)); Assert.assertEquals(0, buf.position()); @@ -111,87 +107,46 @@ public void testBehaviorWhenReportedSizesLargeAndExceptionIgnored() throws Excep Assert.assertEquals(4, buf.capacity()); Assert.assertEquals(i, buf.getInt()); } - mapper.close(); - } - finally { - for (File file : baseDir.listFiles()) { - file.delete(); - } } } @Test public void testBehaviorWhenReportedSizesSmall() throws Exception { - File baseDir = Files.createTempDir(); - baseDir.deleteOnExit(); - - try { - FileSmoosher smoosher = new FileSmoosher(baseDir, 21); - final SmooshedWriter writer = smoosher.addWithSmooshedWriter("1", 2); - boolean exceptionThrown = false; - try { - writer.write(ByteBuffer.wrap(Ints.toByteArray(1))); - } - catch (ISE e) { - Assert.assertTrue(e.getMessage().contains("Liar!!!")); - exceptionThrown = true; - } + File baseDir = folder.newFolder("base"); - Assert.assertTrue(exceptionThrown); - File[] files = baseDir.listFiles(); - Assert.assertEquals(1, files.length); - Assert.assertEquals(0, files[0].length()); + FileSmoosher smoosher = new FileSmoosher(baseDir, 21); + boolean exceptionThrown = false; + try (final SmooshedWriter writer = smoosher.addWithSmooshedWriter("1", 2)) { + writer.write(ByteBuffer.wrap(Ints.toByteArray(1))); } - finally { - for (File file : baseDir.listFiles()) { - file.delete(); - } + catch (ISE e) { + Assert.assertTrue(e.getMessage().contains("Liar!!!")); + exceptionThrown = true; } + + Assert.assertTrue(exceptionThrown); + File[] files = baseDir.listFiles(); + Assert.assertEquals(1, files.length); + Assert.assertEquals(0, files[0].length()); } @Test public void testDeterministicFileUnmapping() throws IOException { - File baseDir = Files.createTempDir(); - baseDir.deleteOnExit(); + File baseDir = folder.newFolder("base"); int fileSize = 1 << 20; // 1 MB - try { - long totalMemoryUsedBeforeAddingFile = totalMemoryUsedByDirectAndMappedBuffers(); - FileSmoosher smoosher = new FileSmoosher(baseDir); - File dataFile = createTempFileOfSize("data", "bin", fileSize); - smoosher.add(dataFile); - // In case smoosher maps some own files internally (though currently it is not), let it unmap them - smoosher.close(); - long totalMemoryUsedAfterAddingFile = totalMemoryUsedByDirectAndMappedBuffers(); - // Assert no hanging file mappings left by either smoosher or smoosher.add(file) - Assert.assertEquals(totalMemoryUsedBeforeAddingFile, totalMemoryUsedAfterAddingFile); - } - finally { - for (File file : baseDir.listFiles()) { - file.delete(); + long totalMemoryUsedBeforeAddingFile = BufferUtils.totalMemoryUsedByDirectAndMappedBuffers(); + try (FileSmoosher smoosher = new FileSmoosher(baseDir)) { + File dataFile = folder.newFile("data.bin"); + try (RandomAccessFile raf = new RandomAccessFile(dataFile, "rw")) { + raf.setLength(fileSize); } + smoosher.add(dataFile); } - } - - private static File createTempFileOfSize(String prefix, String suffix, int fileSize) throws IOException - { - File dataFile = File.createTempFile(prefix, suffix); - dataFile.deleteOnExit(); - try (RandomAccessFile raf = new RandomAccessFile(dataFile, "rw")) { - raf.setLength(fileSize); - } - return dataFile; - } - - private static long totalMemoryUsedByDirectAndMappedBuffers() - { - long totalMemoryUsed = 0L; - List pools = ManagementFactory.getPlatformMXBeans(BufferPoolMXBean.class); - for (BufferPoolMXBean pool : pools) { - totalMemoryUsed += pool.getMemoryUsed(); - } - return totalMemoryUsed; + long totalMemoryUsedAfterAddingFile = BufferUtils.totalMemoryUsedByDirectAndMappedBuffers(); + // Assert no hanging file mappings left by either smoosher or smoosher.add(file) + Assert.assertEquals(totalMemoryUsedBeforeAddingFile, totalMemoryUsedAfterAddingFile); } } From c5c30ed11e1409e2e9134037fdc966093ceab3ea Mon Sep 17 00:00:00 2001 From: leventov Date: Tue, 20 Sep 2016 22:33:43 +0300 Subject: [PATCH 6/7] Minor fixes --- src/test/java/com/metamx/common/FileUtilsTest.java | 2 +- .../com/metamx/common/io/smoosh/SmooshedFileMapperTest.java | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/metamx/common/FileUtilsTest.java b/src/test/java/com/metamx/common/FileUtilsTest.java index b3c32604..c523fea1 100644 --- a/src/test/java/com/metamx/common/FileUtilsTest.java +++ b/src/test/java/com/metamx/common/FileUtilsTest.java @@ -35,11 +35,11 @@ public class FileUtilsTest public void testMap() throws IOException { File dataFile = folder.newFile("data"); + long buffersMemoryBefore = BufferUtils.totalMemoryUsedByDirectAndMappedBuffers(); try (RandomAccessFile raf = new RandomAccessFile(dataFile, "rw")) { raf.write(42); raf.setLength(1 << 20); // 1 MB } - long buffersMemoryBefore = BufferUtils.totalMemoryUsedByDirectAndMappedBuffers(); try (MappedByteBufferHandler mappedByteBufferHandler = FileUtils.map(dataFile)) { Assert.assertEquals(42, mappedByteBufferHandler.get().get(0)); } diff --git a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java index 129b17df..23a4dd11 100644 --- a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java +++ b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java @@ -136,12 +136,11 @@ public void testDeterministicFileUnmapping() throws IOException { File baseDir = folder.newFolder("base"); - int fileSize = 1 << 20; // 1 MB long totalMemoryUsedBeforeAddingFile = BufferUtils.totalMemoryUsedByDirectAndMappedBuffers(); try (FileSmoosher smoosher = new FileSmoosher(baseDir)) { File dataFile = folder.newFile("data.bin"); try (RandomAccessFile raf = new RandomAccessFile(dataFile, "rw")) { - raf.setLength(fileSize); + raf.setLength(1 << 20); // 1 MB } smoosher.add(dataFile); } From 3e06ed5f35bfbd0d6c4d162063cd09d14837748b Mon Sep 17 00:00:00 2001 From: leventov Date: Wed, 21 Sep 2016 21:09:38 +0300 Subject: [PATCH 7/7] Close FileSmoosher in testBehaviorWhenReportedSizesSmall() --- .../io/smoosh/SmooshedFileMapperTest.java | 26 +++++++++---------- 1 file changed, 13 insertions(+), 13 deletions(-) diff --git a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java index 23a4dd11..482429cc 100644 --- a/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java +++ b/src/test/java/com/metamx/common/io/smoosh/SmooshedFileMapperTest.java @@ -115,20 +115,20 @@ public void testBehaviorWhenReportedSizesSmall() throws Exception { File baseDir = folder.newFolder("base"); - FileSmoosher smoosher = new FileSmoosher(baseDir, 21); - boolean exceptionThrown = false; - try (final SmooshedWriter writer = smoosher.addWithSmooshedWriter("1", 2)) { - writer.write(ByteBuffer.wrap(Ints.toByteArray(1))); - } - catch (ISE e) { - Assert.assertTrue(e.getMessage().contains("Liar!!!")); - exceptionThrown = true; - } + try (FileSmoosher smoosher = new FileSmoosher(baseDir, 21)) { + boolean exceptionThrown = false; + try (final SmooshedWriter writer = smoosher.addWithSmooshedWriter("1", 2)) { + writer.write(ByteBuffer.wrap(Ints.toByteArray(1))); + } catch (ISE e) { + Assert.assertTrue(e.getMessage().contains("Liar!!!")); + exceptionThrown = true; + } - Assert.assertTrue(exceptionThrown); - File[] files = baseDir.listFiles(); - Assert.assertEquals(1, files.length); - Assert.assertEquals(0, files[0].length()); + Assert.assertTrue(exceptionThrown); + File[] files = baseDir.listFiles(); + Assert.assertEquals(1, files.length); + Assert.assertEquals(0, files[0].length()); + } } @Test