MVStore bugfixes and test cases

h2/src/main/org/h2/mvstore/MVMap.java

@@ -527,9 +527,11 @@ public class MVMap<K, V> extends AbstractMap<K, V>
     }
 
     /**
-     * Close the map, making it read only and release the memory.
+     * Close the map, making it read only and release the memory. This method
+     * may only be called when closing the store or when removing the map, as
+     * further writes are not possible.
      */
-    public void close() {
+    void close() {
         closed = true;
         readOnly = true;
         removeAllOldVersions();
@@ -1162,10 +1164,6 @@ public class MVMap<K, V> extends AbstractMap<K, V>
     }
 
     void setWriteVersion(long writeVersion) {
-        if (readOnly) {
-            throw DataUtils.newIllegalStateException(
-                    DataUtils.ERROR_INTERNAL, "Trying to write to a read-only map");
-        }
         this.writeVersion = writeVersion;
     }
 

h2/src/main/org/h2/mvstore/MVStore.java

@@ -15,9 +15,9 @@ import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Iterator;
 import java.util.Map;
-import java.util.Map.Entry;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
+
 import org.h2.compress.CompressLZF;
 import org.h2.compress.Compressor;
 import org.h2.mvstore.cache.CacheLongKeyLIRS;
@@ -268,7 +268,7 @@ public class MVStore {
         o = config.get("cacheSize");
         int mb = o == null ? 16 : (Integer) o;
         int maxMemoryBytes = mb * 1024 * 1024;
-        int averageMemory = pageSplitSize / 2;
+        int averageMemory = Math.max(10, pageSplitSize / 2);
         int segmentCount = 16;
         int stackMoveDistance = maxMemoryBytes / averageMemory * 2 / 100;
         cache = new CacheLongKeyLIRS<Page>(
@@ -642,6 +642,8 @@ public class MVStore {
      * Close the file and the store. If there are any committed but unsaved
      * changes, they are written to disk first. If any temporary data was
      * written but not committed, this is rolled back. All open maps are closed.
+     * <p>
+     * It is not allowed to concurrently call close and store.
      */
     public void close() {
         if (closed) {
@@ -649,26 +651,12 @@ public class MVStore {
         }
         if (fileStore != null && !fileStore.isReadOnly()) {
             stopBackgroundThread();
-
-            ; // TODO this is used for testing only
-            // wait until a pending store operation is finished
+            
             if (currentStoreVersion >= 0) {
-                System.out.println("Currently storing, waiting...");
-                Map<Thread, StackTraceElement[]> st = Thread.getAllStackTraces();
-                for(Entry<Thread, StackTraceElement[]> e : st.entrySet()) {
-                    System.out.println(e.getKey().toString());
-                    System.out.println(Arrays.toString(e.getValue()));
-                }
-                for (int i=0; i<10000 && currentStoreVersion >= 0; i++) {
-                    try {
-                        Thread.sleep(1);
-                    } catch (InterruptedException e) {
-                        // ignore
-                    }
-                }           
-                if (currentStoreVersion >= 0) {
-                    System.out.println("Still storing?!");
-                }
+                // in this case, store is called manually in another thread
+                throw DataUtils.newIllegalStateException(
+                        DataUtils.ERROR_WRITING_FAILED, 
+                        "Can not close while storing");
             }
             
             if (hasUnsavedChanges() || lastCommittedVersion != currentVersion) {
@@ -827,8 +815,18 @@ public class MVStore {
             throw DataUtils.newIllegalStateException(
                     DataUtils.ERROR_WRITING_FAILED, "This store is read-only");
         }
+        try {
+            currentStoreVersion = currentVersion;
+            return storeNow(temp);
+        } finally {
+            // in any case reset the current store version,
+            // to allow closing the store
+            currentStoreVersion = -1;
+        }
+    }
+ 
+    private long storeNow(boolean temp) {
         int currentUnsavedPageCount = unsavedPageCount;
-        currentStoreVersion = currentVersion;
         long storeVersion = currentStoreVersion;
         long version = ++currentVersion;
         long time = getTime();
@@ -911,7 +909,8 @@ public class MVStore {
 
         // this will (again) modify maxLengthLive, but
         // the correct value is written in the chunk header
-        buff = meta.getRoot().writeUnsavedRecursive(c, buff);
+        Page metaRoot = meta.getRoot();
+        buff = metaRoot.writeUnsavedRecursive(c, buff);
 
         int chunkLength = buff.position();
 
@@ -941,7 +940,7 @@ public class MVStore {
 
         c.start = filePos;
         c.length = chunkLength;
-        c.metaRootPos = meta.getRoot().getPos();
+        c.metaRootPos = metaRoot.getPos();
         buff.position(0);
         c.writeHeader(buff);
         rootChunkStart = filePos;
@@ -970,11 +969,10 @@ public class MVStore {
                 p.writeEnd();
             }
         }
-        meta.getRoot().writeEnd();
+        metaRoot.writeEnd();
 
         // some pages might have been changed in the meantime (in the newest version)
         unsavedPageCount = Math.max(0, unsavedPageCount - currentUnsavedPageCount);
-        currentStoreVersion = -1;
         if (!temp) {
             metaChanged = false;
             lastStoredVersion = storeVersion;
@@ -1428,6 +1426,10 @@ public class MVStore {
      * @return the page
      */
     Page readPage(MVMap<?, ?> map, long pos) {
+        if (pos == 0) {
+            throw DataUtils.newIllegalStateException(
+                    DataUtils.ERROR_FILE_CORRUPT, "Position 0");
+        }
         Page p = cache.get(pos);
         if (p == null) {
             Chunk c = getChunk(pos);

h2/src/main/org/h2/mvstore/Page.java

@@ -871,6 +871,10 @@ public class Page {
             for (int i = 0; i < len; i++) {
                 Page p = childrenPages[i];
                 if (p != null) {
+                    if (p.getPos() == 0) {
+                        throw DataUtils.newIllegalStateException(
+                                DataUtils.ERROR_INTERNAL, "Page not written");
+                    }
                     p.writeEnd();
                     childrenPages[i] = null;
                 }

h2/src/test/org/h2/test/store/TestConcurrent.java

@@ -16,6 +16,7 @@ import java.util.Map;
 import java.util.Random;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import org.h2.mvstore.DataUtils;
 import org.h2.mvstore.MVMap;
 import org.h2.mvstore.MVMapConcurrent;
 import org.h2.mvstore.MVStore;
@@ -43,12 +44,45 @@ public class TestConcurrent extends TestMVStore {
         FileUtils.deleteRecursive(getBaseDir(), true);
         FileUtils.createDirectories(getBaseDir());
 
+        testConcurrentStoreAndClose();
         testConcurrentOnlineBackup();
         testConcurrentMap();
         testConcurrentIterate();
         testConcurrentWrite();
         testConcurrentRead();
     }
+    
+    private void testConcurrentStoreAndClose() throws InterruptedException {
+        String fileName = getBaseDir() + "/testConcurrentStoreAndClose.h3";
+        final MVStore s = openStore(fileName);
+        Task task = new Task() {
+            @Override
+            public void call() throws Exception {
+                int x = 0;
+                while (!stop) {
+                    s.setStoreVersion(x++);
+                    s.store();
+                }
+            }
+        };
+        task.execute();
+        Thread.sleep(1);
+        try {
+            s.close();
+            // sometimes closing works, in which case
+            // storing fails at some point
+            Thread.sleep(1000);
+            Exception e = task.getException();
+            assertEquals(DataUtils.ERROR_CLOSED, 
+                    DataUtils.getErrorCode(e.getMessage()));
+        } catch (IllegalStateException e) {
+            // sometimes storing works, in which case
+            // closing fails
+            assertEquals(DataUtils.ERROR_WRITING_FAILED, 
+                    DataUtils.getErrorCode(e.getMessage()));
+        }
+        s.close();
+    }
 
     /**
      * Test the concurrent map implementation.
@@ -105,8 +139,7 @@ public class TestConcurrent extends TestMVStore {
     }
 
     private void testConcurrentOnlineBackup() throws Exception {
-        // need to use NIO because we mix absolute and relative operations
-        String fileName = "nio:" + getBaseDir() + "/onlineBackup.h3";
+        String fileName = getBaseDir() + "/onlineBackup.h3";
         String fileNameRestore = getBaseDir() + "/onlineRestore.h3";
         final MVStore s = openStore(fileName);
         final MVMap<Integer, byte[]> map = s.openMap("test");

h2/src/test/org/h2/test/store/TestMVStore.java

@@ -5,7 +5,6 @@
  */
 package org.h2.test.store;
 
-import java.io.PrintWriter;
 import java.lang.Thread.UncaughtExceptionHandler;
 import java.nio.ByteBuffer;
 import java.nio.channels.FileChannel;
@@ -17,9 +16,9 @@ import java.util.concurrent.atomic.AtomicReference;
 
 import org.h2.mvstore.Cursor;
 import org.h2.mvstore.DataUtils;
+import org.h2.mvstore.FileStore;
 import org.h2.mvstore.MVMap;
 import org.h2.mvstore.MVStore;
-import org.h2.mvstore.MVStoreTool;
 import org.h2.mvstore.OffHeapStore;
 import org.h2.mvstore.type.DataType;
 import org.h2.mvstore.type.ObjectDataType;
@@ -49,6 +48,7 @@ public class TestMVStore extends TestBase {
     public void test() throws Exception {
         FileUtils.deleteRecursive(getBaseDir(), true);
         FileUtils.createDirectories(getBaseDir());
+        testRemoveMap();
         testIsEmpty();
         testOffHeapStorage();
         testNewerWriteVersion();
@@ -66,10 +66,7 @@ public class TestMVStore extends TestBase {
         testCacheSize();
         testConcurrentOpen();
         testFileHeader();
-        
-        int todoFixTestCase;
-//        testFileHeaderCorruption();
-        
+        testFileHeaderCorruption();
         testIndexSkip();
         testMinMaxNextKey();
         testStoreVersion();
@@ -100,6 +97,30 @@ public class TestMVStore extends TestBase {
         testLargerThan2G();
     }
     
+    private void testRemoveMap() throws Exception {
+        String fileName = getBaseDir() + "/testCloseMap.h3";
+        FileUtils.delete(fileName);
+        MVStore s = new MVStore.Builder().
+            fileName(fileName).
+            open();
+        MVMap<Integer, Integer> map;
+        
+        map = s.openMap("data");
+        map.put(1, 1);
+        assertEquals(1, map.get(1).intValue());
+        s.store();
+        
+        map.removeMap();
+        s.store();
+        
+        map = s.openMap("data");
+        assertTrue(map.isEmpty());
+        map.put(2, 2);
+        s.store();
+        
+        s.close();
+    }
+    
     private void testIsEmpty() throws Exception {
         MVStore s = new MVStore.Builder().
                 pageSplitSize(50).
@@ -600,25 +621,34 @@ public class TestMVStore extends TestBase {
     private void testFileHeaderCorruption() throws Exception {
         String fileName = getBaseDir() + "/testFileHeader.h3";
         MVStore s = openStore(fileName);
-        s.setRetentionTime(10);
-        MVMap<Integer, Integer> map = s.openMap("test");
-        for (int i = 0; i < 5; i++) {
-            s.setStoreVersion(i);
+        s.setRetentionTime(0);
+        MVMap<Integer, byte[]> map;
+        map = s.openMap("test");
+        map.put(0, new byte[100]);
+        for (int i = 0; i < 10; i++) {
+            map = s.openMap("test" + i);
+            map.put(0, new byte[1000]);
             s.store();
         }
-        // ensure the oldest chunks can be overwritten
-        Thread.sleep(11);
-        s.compact(50);
-        map.put(10, 100);
-        s.store();
-        FilePath f = FilePath.get(fileName);
+        FileStore fs = s.getFileStore();
+        long size = fs.getFile().size();
+        for (int i = 0; i < 10; i++) {
+            map = s.openMap("test" + i);
+            map.removeMap();
+            s.store();
+            s.commit();
+            s.compact(100);
+            if (fs.getFile().size() <= size) {
+                break;
+            }
+        }
         s.close();
+        
+        FilePath f = FilePath.get(fileName);
         int blockSize = 4 * 1024;
         // test corrupt file headers
         for (int i = 0; i <= blockSize; i += blockSize) {
             FileChannel fc = f.open("rw");
-MVStoreTool.dump(fileName, new PrintWriter(System.out));            
-            
             if (i == 0) {
                 // corrupt the last block (the end header)
                 fc.truncate(fc.size() - 4096);
@@ -635,14 +665,12 @@ MVStoreTool.dump(fileName, new PrintWriter(System.out));
             fc.write(buff, i);
             fc.close();
 
-MVStoreTool.dump(fileName, new PrintWriter(System.out));            
-            
             if (i == 0) {
                 // if the first header is corrupt, the second
                 // header should be used
                 s = openStore(fileName);
                 map = s.openMap("test");
-                assertEquals(100, map.get(10).intValue());
+                assertEquals(100, map.get(0).length);
                 s.close();
             } else {
                 // both headers are corrupt
@@ -906,7 +934,6 @@ MVStoreTool.dump(fileName, new PrintWriter(System.out));
         assertEquals("Hello", oldMap.get(1));
         // System.out.println(oldMap.get(2));
         assertEquals("World", oldMap.get(2));
-        oldMap.close();
 
         // print the newest version ("Hi")
         // System.out.println(map.get(1));

h2/src/test/org/h2/test/store/TestRandomMapOps.java

@@ -170,8 +170,10 @@ public class TestRandomMapOps extends TestBase {
     }
     
     private static MVStore openStore(String fileName) {
-        return new MVStore.Builder().fileName(fileName).
+        MVStore s = new MVStore.Builder().fileName(fileName).
                 pageSplitSize(50).writeDelay(0).open();    
+        s.setRetentionTime(0);
+        return s;
     }
     
     private void assertEqualsMapValues(byte[] x, byte[] y) {