You are viewing a plain text version of this content. The canonical link for it is here.
Posted to xindice-dev@xml.apache.org by vg...@apache.org on 2007/03/23 23:58:30 UTC

svn commit: r521932 - in /xml/xindice/trunk/java: src/org/apache/xindice/core/filer/HashFiler.java src/org/apache/xindice/core/filer/Paged.java src/org/apache/xindice/server/XindiceServlet.java tests/src/org/apache/xindice/core/filer/FilerTestBase.java

Author: vgritsenko
Date: Fri Mar 23 15:58:29 2007
New Revision: 521932

URL: http://svn.apache.org/viewvc?view=rev&rev=521932
Log:
fix rarely happening error in paged

Modified:
    xml/xindice/trunk/java/src/org/apache/xindice/core/filer/HashFiler.java
    xml/xindice/trunk/java/src/org/apache/xindice/core/filer/Paged.java
    xml/xindice/trunk/java/src/org/apache/xindice/server/XindiceServlet.java
    xml/xindice/trunk/java/tests/src/org/apache/xindice/core/filer/FilerTestBase.java

Modified: xml/xindice/trunk/java/src/org/apache/xindice/core/filer/HashFiler.java
URL: http://svn.apache.org/viewvc/xml/xindice/trunk/java/src/org/apache/xindice/core/filer/HashFiler.java?view=diff&rev=521932&r1=521931&r2=521932
==============================================================================
--- xml/xindice/trunk/java/src/org/apache/xindice/core/filer/HashFiler.java (original)
+++ xml/xindice/trunk/java/src/org/apache/xindice/core/filer/HashFiler.java Fri Mar 23 15:58:29 2007
@@ -165,6 +165,7 @@
                     break;
                 }
 
+                // Check the chain
                 long pageNum = ph.getNextCollision();
                 if (pageNum == NO_PAGE) {
                     // Reached end of chain, add new page
@@ -179,7 +180,7 @@
                     break;
                 }
 
-                // Go to next page in chain
+                // Go to the next page in chain
                 p = getPage(pageNum);
             }
 
@@ -192,10 +193,6 @@
             }
             ph.setModified(t);
             ph.setStatus(RECORD);
-
-            // Write modifications to the page header before exiting synchronization block
-            // This will prevent other threads from getting this same page
-            p.write();
         }
 
         return p;
@@ -217,7 +214,7 @@
             writeValue(p, value);
             p = null;
         } catch (Exception e) {
-            throw new FilerException(FaultCodes.DBE_CANNOT_CREATE, "Exception: " + e);
+            throw new FilerException(FaultCodes.DBE_CANNOT_CREATE, "Exception: " + e, e);
         } finally {
             // FIXME It's not enough. At this point, new record could have been added to the chain
             if (p != null) {
@@ -269,7 +266,7 @@
             long pageNum = hash % fileHeader.getPageCount();
 
             Page page = getPage(pageNum);
-            synchronized(page) {
+            synchronized (page) {
                 HashPageHeader prevHead = null;
                 HashPageHeader pageHead;
 

Modified: xml/xindice/trunk/java/src/org/apache/xindice/core/filer/Paged.java
URL: http://svn.apache.org/viewvc/xml/xindice/trunk/java/src/org/apache/xindice/core/filer/Paged.java?view=diff&rev=521932&r1=521931&r2=521932
==============================================================================
--- xml/xindice/trunk/java/src/org/apache/xindice/core/filer/Paged.java (original)
+++ xml/xindice/trunk/java/src/org/apache/xindice/core/filer/Paged.java Fri Mar 23 15:58:29 2007
@@ -162,16 +162,22 @@
      */
     private Configuration config;
 
-    // TODO: This is not a cache right now, but a way to assure that only one page instance at most exists in memory at all times.
     /**
-     * Cache of recently read pages.
+     * Map of pages in use. Guarantees that page with same number will be loaded
+     * into memory just once, allowing to synchronize on page objects to guarantee
+     * no two threads are writing into same page at once.
      *
-     * Cache contains weak references to the Page objects, keys are page numbers (Long objects).
-     * Access synchronized by this map itself.
+     * <p>Map contains weak references to the Page objects, keys are pages themselves.
+     * Access is synchronized by the {@link #pagesLock}.
      */
     private final Map pages = new WeakHashMap();
 
     /**
+     * Lock for synchronizing access to the {@link #pages} map.
+     */
+    private final Object pagesLock = new Object();
+
+    /**
      * Cache of modified pages waiting to be written out.
      * Access is synchronized by the {@link #dirtyLock}.
      */
@@ -332,19 +338,22 @@
      * @throws IOException if an Exception occurs
      */
     protected final Page getPage(long pageNum) throws IOException {
-        final Long lp = new Long(pageNum);
+        final Page lp = new Page(new Long(pageNum));
         Page p = null;
-        synchronized (pages) {
+        synchronized (pagesLock) {
             // Check if page is already loaded in the page cache
             WeakReference ref = (WeakReference) pages.get(lp);
             if (ref != null) {
                 p = (Page) ref.get();
+                // Fall through to p.read(). Even if page present in the pages
+                // map, it still has to be read - it could be that it was just
+                // added to the map but read() was not called yet.
             }
 
-            // If not found, create it and add it to the page cache
+            // If not found, create it and add it to the pages cache
             if (p == null) {
-                p = new Page(lp);
-                pages.put(p.pageNum, new WeakReference(p));
+                p = new Page(new Long(pageNum));
+                pages.put(p, new WeakReference(p));
             }
         }
 
@@ -1249,7 +1258,7 @@
         private int dataPos;
 
 
-        public Page(Long pageNum) {
+        private Page(Long pageNum) {
             this.header = createPageHeader();
             this.pageNum = pageNum;
             this.offset = fileHeader.headerSize + (pageNum.longValue() * fileHeader.pageSize);
@@ -1366,6 +1375,33 @@
         // No synchronization: pageNum is final.
         public int compareTo(Object o) {
             return (int) (pageNum.longValue() - ((Page) o).pageNum.longValue());
+        }
+
+        /**
+         * Return page hash code, which is hash code of its {@link #pageNum}.
+         *
+         * @return Page hash code
+         */
+        public int hashCode() {
+            return pageNum.hashCode();
+        }
+
+        /**
+         * Pages are equal if they are the same or have equal pageNum.
+         *
+         * @param obj Another page
+         * @return true if pages are equal
+         */
+        public boolean equals(Object obj) {
+            if (obj == this) {
+                return true;
+            }
+
+            if (obj instanceof Page) {
+                return pageNum.longValue() == ((Page) obj).pageNum.longValue();
+            }
+
+            return false;
         }
     }
 }

Modified: xml/xindice/trunk/java/src/org/apache/xindice/server/XindiceServlet.java
URL: http://svn.apache.org/viewvc/xml/xindice/trunk/java/src/org/apache/xindice/server/XindiceServlet.java?view=diff&rev=521932&r1=521931&r2=521932
==============================================================================
--- xml/xindice/trunk/java/src/org/apache/xindice/server/XindiceServlet.java (original)
+++ xml/xindice/trunk/java/src/org/apache/xindice/server/XindiceServlet.java Fri Mar 23 15:58:29 2007
@@ -229,7 +229,7 @@
 
             String path = servletConfig.getInitParameter(Xindice.PROP_XINDICE_CONFIGURATION);
             if (path != null) {
-                InputStream inputStream = null;
+                InputStream inputStream;
                 if (path.startsWith("/")) {
                     // Absolute file path
                     log.debug("Loading configuration from filesystem path " + path);
@@ -244,10 +244,7 @@
                 try {
                     configurationDocument = DOMParser.toDocument(inputStream);
                 } finally {
-                    try {
-                        inputStream.close();
-                    } catch (IOException ignored) {
-                    }
+                    inputStream.close();
                 }
             } else {
                 log.debug("Loading the standard configuration");

Modified: xml/xindice/trunk/java/tests/src/org/apache/xindice/core/filer/FilerTestBase.java
URL: http://svn.apache.org/viewvc/xml/xindice/trunk/java/tests/src/org/apache/xindice/core/filer/FilerTestBase.java?view=diff&rev=521932&r1=521931&r2=521932
==============================================================================
--- xml/xindice/trunk/java/tests/src/org/apache/xindice/core/filer/FilerTestBase.java (original)
+++ xml/xindice/trunk/java/tests/src/org/apache/xindice/core/filer/FilerTestBase.java Fri Mar 23 15:58:29 2007
@@ -88,10 +88,11 @@
         if (filer != null) {
             filer.close();
         }
+        filer = null;
 
         String[] files = ROOT.list();
         for (int i = 0; i < files.length; i++) {
-            new File(ROOT, files[i]).delete();
+            assertTrue(new File(ROOT, files[i]).delete());
         }
         ROOT.delete();
         super.tearDown();
@@ -286,8 +287,9 @@
     }
 
     public void testConcurrentInsert() throws Exception {
-        assertTrue(filer.getRecordCount() == 0);
+        assertEquals(0, filer.getRecordCount());
 
+        final Object go = new Object();
         final int THREADS = 30;
         final int ITERATIONS = 65;
 
@@ -296,6 +298,12 @@
             final int threadID = i;
             threads[i] = new Thread() {
                 public void run() {
+                    synchronized (go) {
+                        try {
+                            go.wait();
+                        } catch (InterruptedException e) { /* ignored */ }
+                    }
+
                     for (int ii = 0; ii < ITERATIONS; ii++) {
                         Key key = new Key("T" + threadID + "I" + ii);
                         Value value = new Value("<test thread=\"" + threadID + "\" iteration=\"" + ii + "\"/>");
@@ -305,26 +313,24 @@
                             e.printStackTrace();
                         }
                     }
-                    // System.out.println(getName() + " done.");
+                    // System.out.println(this.getName() + " done.");
                 }
             };
             threads[i].setName("FilerTest" + i);
+            threads[i].start();
         }
 
         // Start all the threads at once
-        for (int i = 0; i < THREADS; i++) {
-            threads[i].start();
+        Thread.sleep(250);
+        synchronized (go) {
+            go.notifyAll();
         }
-        Thread.sleep(1000);
-
         for (int i = 0; i < THREADS; i++) {
             threads[i].join();
         }
 
-        filer.flush();
-
         // Check results
-        assertEquals(filer.getRecordCount(), THREADS * ITERATIONS);
+        assertEquals(THREADS * ITERATIONS, filer.getRecordCount());
         for (int i = 0; i < THREADS; i++) {
             for (int ii = 0; ii < ITERATIONS; ii++) {
                 Key key = new Key("T" + i + "I" + ii);