From a69181e8f631d9aa54908bfcebdc0054cbe96543 Mon Sep 17 00:00:00 2001 From: Andrew Snare Date: Wed, 27 Aug 2014 11:28:42 +0200 Subject: [PATCH 1/2] Discard the reference to the underlying buffer during close(), and throw ClosedDatabaseException on subsequent database access. Currently clients can continue to access the database even after it has been closed. This prevents that, and should also help ensure the underlying database is garbage collected. --- .../maxmind/db/ClosedDatabaseException.java | 15 ++++++++++++ src/main/java/com/maxmind/db/Reader.java | 19 +++++++++++---- src/test/java/com/maxmind/db/ReaderTest.java | 24 ++++++++++++++----- 3 files changed, 47 insertions(+), 11 deletions(-) create mode 100644 src/main/java/com/maxmind/db/ClosedDatabaseException.java diff --git a/src/main/java/com/maxmind/db/ClosedDatabaseException.java b/src/main/java/com/maxmind/db/ClosedDatabaseException.java new file mode 100644 index 00000000..6a02c28a --- /dev/null +++ b/src/main/java/com/maxmind/db/ClosedDatabaseException.java @@ -0,0 +1,15 @@ +package com.maxmind.db; + +import java.io.IOException; + +/** + * Signals that the underlying database has been closed. + */ +public class ClosedDatabaseException extends IOException { + + private static final long serialVersionUID = 1L; + + ClosedDatabaseException() { + super("The MaxMind DB has been closed."); + } +} diff --git a/src/main/java/com/maxmind/db/Reader.java b/src/main/java/com/maxmind/db/Reader.java index 9b69f903..f1658be5 100644 --- a/src/main/java/com/maxmind/db/Reader.java +++ b/src/main/java/com/maxmind/db/Reader.java @@ -6,6 +6,7 @@ import java.io.InputStream; import java.net.InetAddress; import java.nio.ByteBuffer; +import java.util.concurrent.atomic.AtomicReference; import com.fasterxml.jackson.databind.JsonNode; @@ -21,7 +22,7 @@ public final class Reader implements Closeable { private final int ipV4Start; private final Metadata metadata; - private final BufferHolder bufferHolder; + private final AtomicReference bufferHolderReference; /** * The file mode to use when opening a MaxMind DB. @@ -81,9 +82,9 @@ public Reader(File database, FileMode fileMode) throws IOException { } private Reader(BufferHolder bufferHolder, String name) throws IOException { - this.bufferHolder = bufferHolder; + this.bufferHolderReference = new AtomicReference(bufferHolder); - ByteBuffer buffer = this.bufferHolder.get(); + ByteBuffer buffer = bufferHolder.get(); int start = this.findMetadataStart(buffer, name); Decoder metadataDecoder = new Decoder(buffer, start); @@ -102,7 +103,7 @@ private Reader(BufferHolder bufferHolder, String name) throws IOException { * if a file I/O error occurs. */ public JsonNode get(InetAddress ipAddress) throws IOException { - ByteBuffer buffer = this.bufferHolder.get(); + ByteBuffer buffer = getBufferHolder().get(); int pointer = this.findAddressInTree(buffer, ipAddress); if (pointer == 0) { return null; @@ -110,6 +111,14 @@ public JsonNode get(InetAddress ipAddress) throws IOException { return this.resolveDataPointer(buffer, pointer); } + private BufferHolder getBufferHolder() throws ClosedDatabaseException { + BufferHolder bufferHolder = bufferHolderReference.get(); + if (bufferHolder == null) { + throw new ClosedDatabaseException(); + } + return bufferHolder; + } + private int findAddressInTree(ByteBuffer buffer, InetAddress address) throws InvalidDatabaseException { byte[] rawAddress = address.getAddress(); @@ -241,6 +250,6 @@ Metadata getMetadata() { */ @Override public void close() throws IOException { - this.bufferHolder.close(); + bufferHolderReference.getAndSet(null).close(); } } diff --git a/src/test/java/com/maxmind/db/ReaderTest.java b/src/test/java/com/maxmind/db/ReaderTest.java index 85ed2311..4a30c080 100644 --- a/src/test/java/com/maxmind/db/ReaderTest.java +++ b/src/test/java/com/maxmind/db/ReaderTest.java @@ -37,15 +37,14 @@ public void test() throws IOException, URISyntaxException { Reader reader = new Reader(new File(file)); try { this.testMetadata(reader, ipVersion, recordSize); + if (ipVersion == 4) { + this.testIpV4(reader, file); + } else { + this.testIpV6(reader, file); + } } finally { reader.close(); } - - if (ipVersion == 4) { - this.testIpV4(reader, file); - } else { - this.testIpV6(reader, file); - } } } } @@ -290,6 +289,19 @@ private void testBrokenDataPointer(Reader reader) throws IOException, reader.get(InetAddress.getByName("1.1.1.16")); } + @Test + public void testClosedReaderThrowsException() throws IOException, URISyntaxException { + URI file = ReaderTest.class.getResource( + "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb").toURI(); + Reader reader = new Reader(new File(file)); + + this.thrown.expect(ClosedDatabaseException.class); + this.thrown.expectMessage("The MaxMind DB has been closed."); + + reader.close(); + reader.get(InetAddress.getByName("1.1.1.16")); + } + private void testMetadata(Reader reader, int ipVersion, long recordSize) { Metadata metadata = reader.getMetadata(); From 096a20482a30b79f38a53115cc119a9c32d0930d Mon Sep 17 00:00:00 2001 From: Andrew Snare Date: Wed, 27 Aug 2014 11:45:56 +0200 Subject: [PATCH 2/2] Ensure all tests close the database reader when finished. --- .../com/maxmind/db/MultiThreadedTest.java | 18 +++++- src/test/java/com/maxmind/db/ReaderTest.java | 64 ++++++++++++------- 2 files changed, 55 insertions(+), 27 deletions(-) diff --git a/src/test/java/com/maxmind/db/MultiThreadedTest.java b/src/test/java/com/maxmind/db/MultiThreadedTest.java index 415ae2c7..a06586a7 100644 --- a/src/test/java/com/maxmind/db/MultiThreadedTest.java +++ b/src/test/java/com/maxmind/db/MultiThreadedTest.java @@ -33,7 +33,11 @@ public JsonNode call() throws UnknownHostException, IOException, "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb") .toURI(); final Reader reader = new Reader(new File(file)); - return reader.get(InetAddress.getByName("::1.1.1.0")); + try { + return reader.get(InetAddress.getByName("::1.1.1.0")); + } finally { + reader.close(); + } } }; MultiThreadedTest.runThreads(task); @@ -45,7 +49,11 @@ public void streamThreadTest() throws IOException, InterruptedException, final Reader reader = new Reader(ReaderTest.class.getResource( "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb") .openStream()); - MultiThreadedTest.threadTest(reader); + try { + MultiThreadedTest.threadTest(reader); + } finally { + reader.close(); + } } @Test @@ -54,7 +62,11 @@ public void mmapThreadTest() throws IOException, InterruptedException, URI file = ReaderTest.class.getResource( "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb").toURI(); final Reader reader = new Reader(new File(file)); - MultiThreadedTest.threadTest(reader); + try { + MultiThreadedTest.threadTest(reader); + } finally { + reader.close(); + } } private static void threadTest(final Reader reader) throws InterruptedException, diff --git a/src/test/java/com/maxmind/db/ReaderTest.java b/src/test/java/com/maxmind/db/ReaderTest.java index 4a30c080..f4a19abb 100644 --- a/src/test/java/com/maxmind/db/ReaderTest.java +++ b/src/test/java/com/maxmind/db/ReaderTest.java @@ -16,6 +16,8 @@ import java.util.HashMap; import java.util.Map; +import org.junit.After; +import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; @@ -27,6 +29,20 @@ public class ReaderTest { private final ObjectMapper om = new ObjectMapper(); + private Reader testReader; + + @Before + public void setupReader() { + testReader = null; + } + + @After + public void teardownReader() throws IOException { + if (testReader != null) { + testReader.close(); + } + } + @Test public void test() throws IOException, URISyntaxException { for (long recordSize : new long[] { 24, 28, 32 }) { @@ -56,8 +72,8 @@ public void testNoIpV4SearchTreeFile() throws IOException, "/maxmind-db/test-data/MaxMind-DB-no-ipv4-search-tree.mmdb") .toURI(); - Reader reader = new Reader(new File(file)); - this.testNoIpV4SearchTree(reader); + testReader = new Reader(new File(file)); + this.testNoIpV4SearchTree(testReader); } @Test @@ -66,8 +82,8 @@ public void testNoIpV4SearchTreeURL() throws IOException, InputStream stream = ReaderTest.class.getResource( "/maxmind-db/test-data/MaxMind-DB-no-ipv4-search-tree.mmdb") .openStream(); - Reader reader = new Reader(stream); - this.testNoIpV4SearchTree(reader); + testReader = new Reader(stream); + this.testNoIpV4SearchTree(testReader); } private void testNoIpV4SearchTree(Reader reader) throws IOException, @@ -84,8 +100,8 @@ public void testDecodingTypesFile() throws URISyntaxException, IOException { URI file = ReaderTest.class.getResource( "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb").toURI(); - Reader reader = new Reader(new File(file)); - this.testDecodingTypes(reader); + testReader = new Reader(new File(file)); + this.testDecodingTypes(testReader); } @Test @@ -94,8 +110,8 @@ public void testDecodingTypesURL() throws URISyntaxException, IOException { "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb") .openStream(); - Reader reader = new Reader(stream); - this.testDecodingTypes(reader); + testReader = new Reader(stream); + this.testDecodingTypes(testReader); } private void testDecodingTypes(Reader reader) throws URISyntaxException, @@ -147,8 +163,8 @@ public void testZerosFile() throws URISyntaxException, IOException { URI file = ReaderTest.class.getResource( "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb").toURI(); - Reader reader = new Reader(new File(file)); - this.testZeros(reader); + testReader = new Reader(new File(file)); + this.testZeros(testReader); } @Test @@ -157,8 +173,8 @@ public void testZerosURL() throws URISyntaxException, IOException { "/maxmind-db/test-data/MaxMind-DB-test-decoder.mmdb") .openStream(); - Reader reader = new Reader(stream); - this.testZeros(reader); + testReader = new Reader(stream); + this.testZeros(testReader); } private void testZeros(Reader reader) throws URISyntaxException, @@ -196,8 +212,8 @@ public void testBrokenDatabaseFile() throws URISyntaxException, IOException { "/maxmind-db/test-data/GeoIP2-City-Test-Broken-Double-Format.mmdb") .toURI(); - Reader reader = new Reader(new File(file)); - this.testBrokenDatabase(reader); + testReader = new Reader(new File(file)); + this.testBrokenDatabase(testReader); } @Test @@ -207,8 +223,8 @@ public void testBrokenDatabaseURL() throws URISyntaxException, IOException { "/maxmind-db/test-data/GeoIP2-City-Test-Broken-Double-Format.mmdb") .openStream(); - Reader reader = new Reader(stream); - this.testBrokenDatabase(reader); + testReader = new Reader(stream); + this.testBrokenDatabase(testReader); } private void testBrokenDatabase(Reader reader) throws URISyntaxException, @@ -229,8 +245,8 @@ public void testBrokenSearchTreePointerFile() throws URISyntaxException, "/maxmind-db/test-data/MaxMind-DB-test-broken-pointers-24.mmdb") .toURI(); - Reader reader = new Reader(new File(file)); - this.testBrokenSearchTreePointer(reader); + testReader = new Reader(new File(file)); + this.testBrokenSearchTreePointer(testReader); } @Test @@ -241,8 +257,8 @@ public void testBrokenSearchTreePointerURL() throws URISyntaxException, "/maxmind-db/test-data/MaxMind-DB-test-broken-pointers-24.mmdb") .openStream(); - Reader reader = new Reader(stream); - this.testBrokenSearchTreePointer(reader); + testReader = new Reader(stream); + this.testBrokenSearchTreePointer(testReader); } private void testBrokenSearchTreePointer(Reader reader) @@ -263,8 +279,8 @@ public void testBrokenDataPointerFile() throws IOException, "/maxmind-db/test-data/MaxMind-DB-test-broken-pointers-24.mmdb") .toURI(); - Reader reader = new Reader(new File(file)); - this.testBrokenDataPointer(reader); + testReader = new Reader(new File(file)); + this.testBrokenDataPointer(testReader); } @Test @@ -275,8 +291,8 @@ public void testBrokenDataPointerURL() throws IOException, "/maxmind-db/test-data/MaxMind-DB-test-broken-pointers-24.mmdb") .openStream(); - Reader reader = new Reader(stream); - this.testBrokenDataPointer(reader); + testReader = new Reader(stream); + this.testBrokenDataPointer(testReader); } private void testBrokenDataPointer(Reader reader) throws IOException,