Fix rare inconsistent read problems surfaced by the more devious Page…

…CacheRule
neo4j · May 20, 2016 · 78611dd · 78611dd
1 parent d277ad8
commit 78611dd
Show file tree

Hide file tree

Showing 29 changed files with 408 additions and 546 deletions.
diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/PageCursor.java
@@ -249,6 +249,14 @@ public abstract class PageCursor implements AutoCloseable
      * ready to be processed. Returns false if there are no more pages to be
      * processed. For instance, if the cursor was requested with PF_NO_GROW
      * and the page most recently processed was the last page in the file.
+     * <p>
+     * <strong>NOTE: When using read locks, read operations can be inconsistent
+     * and may return completely random data. The data returned from a
+     * read-locked page cursor should not be interpreted until after
+     * {@link #shouldRetry()} has returned {@code false}.</strong>
+     * Not interpreting the data also implies that you cannot throw exceptions
+     * from data validation errors until after {@link #shouldRetry()} has told
+     * you that your read was consistent.
      */
     public abstract boolean next() throws IOException;
 
@@ -257,6 +265,14 @@ public abstract class PageCursor implements AutoCloseable
      * and returns true when it is ready to be processed. Returns false if
      * for instance, the cursor was requested with PF_NO_GROW and the page
      * most recently processed was the last page in the file.
+     * <p>
+     * <strong>NOTE: When using read locks, read operations can be inconsistent
+     * and may return completely random data. The data returned from a
+     * read-locked page cursor should not be interpreted until after
+     * {@link #shouldRetry()} has returned {@code false}.</strong>
+     * Not interpreting the data also implies that you cannot throw exceptions
+     * from data validation errors until after {@link #shouldRetry()} has told
+     * you that your read was consistent.
      */
     public abstract boolean next( long pageId ) throws IOException;
 

diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnPageCursor.java
@@ -158,6 +158,7 @@ void clearPageState()
         pointer = victimPage; // make all future page access go to the victim page
         pageSize = 0; // make all future bound checks fail
         page = null; // make all future page navigation fail
+        currentPageId = UNBOUND_PAGE_ID;
     }
 
     @Override

diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnReadPageCursor.java
@@ -53,7 +53,7 @@ public boolean next() throws IOException
     {
         unpinCurrentPage();
         long lastPageId = assertPagedFileStillMappedAndGetIdOfLastPage();
-        if ( nextPageId > lastPageId )
+        if ( nextPageId > lastPageId | nextPageId < 0 )
         {
             return false;
         }
@@ -97,7 +97,8 @@ protected void releaseCursor()
     @Override
     public boolean shouldRetry() throws IOException
     {
-        boolean needsRetry = !page.validateReadLock( lockStamp );
+        MuninnPage p = page;
+        boolean needsRetry = p != null && !p.validateReadLock( lockStamp );
         if ( needsRetry )
         {
             startRetry();

diff --git a/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java b/community/io/src/main/java/org/neo4j/io/pagecache/impl/muninn/MuninnWritePageCursor.java
@@ -54,6 +54,10 @@ public boolean next() throws IOException
     {
         unpinCurrentPage();
         long lastPageId = assertPagedFileStillMappedAndGetIdOfLastPage();
+        if ( nextPageId < 0 )
+        {
+            return false;
+        }
         if ( nextPageId > lastPageId )
         {
             if ( (pf_flags & PagedFile.PF_NO_GROW) != 0 )