jackrabbit-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From thom...@apache.org
Subject svn commit: r631905 - in /jackrabbit/trunk/jackrabbit-core/src: main/java/org/apache/jackrabbit/core/data/ main/java/org/apache/jackrabbit/core/data/db/ main/java/org/apache/jackrabbit/core/persistence/bundle/ test/java/org/apache/jackrabbit/core/data/
Date Thu, 28 Feb 2008 09:47:53 GMT
Author: thomasm
Date: Thu Feb 28 01:47:51 2008
New Revision: 631905

URL: http://svn.apache.org/viewvc?rev=631905&view=rev
Log:
JCR-1414 Data store garbage collection: inUse not correctly synchronized

Added:
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCConcurrentTest.java
  (with props)
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCThread.java
  (with props)
Modified:
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/FileDataStore.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/GarbageCollector.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java
    jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java
    jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/TestAll.java

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/FileDataStore.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/FileDataStore.java?rev=631905&r1=631904&r2=631905&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/FileDataStore.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/FileDataStore.java
Thu Feb 28 01:47:51 2008
@@ -25,8 +25,10 @@
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
+import java.util.Map;
 import java.util.WeakHashMap;
 
 /**
@@ -94,7 +96,7 @@
     /**
      * All data identifiers that are currently in use are in this set until they are garbage
collected.
      */
-    private WeakHashMap inUse = new WeakHashMap();
+    protected Map inUse = Collections.synchronizedMap(new WeakHashMap());
 
     /**
      * Creates a uninitialized data store.
@@ -129,12 +131,14 @@
      */
     public DataRecord getRecord(DataIdentifier identifier) {
         File file = getFile(identifier);
-        if (minModifiedDate != 0 && file.exists() && file.canWrite()) {
-            if (file.lastModified() < minModifiedDate) {
-                file.setLastModified(System.currentTimeMillis());
+        synchronized (this) {
+            if (minModifiedDate != 0 && file.exists() && file.canWrite())
{
+                if (file.lastModified() < minModifiedDate) {
+                    file.setLastModified(System.currentTimeMillis());
+                }
             }
+            usesIdentifier(identifier);
         }
-        usesIdentifier(identifier);
         return new FileDataRecord(identifier, file);
     }
 
@@ -173,38 +177,40 @@
             } finally {
                 output.close();
             }
-
-            // Check if the same record already exists, or
-            // move the temporary file in place if needed
             DataIdentifier identifier = new DataIdentifier(digest.digest());
-            usesIdentifier(identifier);
-            File file = getFile(identifier);
-            File parent = file.getParentFile();
-            if (!parent.isDirectory()) {
-                parent.mkdirs();
-            }
-            if (!file.exists()) {
-                temporary.renameTo(file);
+            File file;
+
+            synchronized (this) {
+                // Check if the same record already exists, or
+                // move the temporary file in place if needed
+                usesIdentifier(identifier);
+                file = getFile(identifier);
+                File parent = file.getParentFile();
+                if (!parent.isDirectory()) {
+                    parent.mkdirs();
+                }
                 if (!file.exists()) {
-                    throw new IOException(
-                            "Can not rename " + temporary.getAbsolutePath()
-                            + " to " + file.getAbsolutePath()
-                            + " (media read only?)");
+                    temporary.renameTo(file);
+                    if (!file.exists()) {
+                        throw new IOException(
+                                "Can not rename " + temporary.getAbsolutePath()
+                                + " to " + file.getAbsolutePath()
+                                + " (media read only?)");
+                    }
+                } else {
+                    long now = System.currentTimeMillis();
+                    if (file.lastModified() < now) {
+                        file.setLastModified(now);
+                    }
                 }
-            } else {
-                long now = System.currentTimeMillis();
-                if (file.lastModified() < now) {
-                    file.setLastModified(now);
+                // Sanity checks on the record file. These should never fail,
+                // but better safe than sorry...
+                if (!file.isFile()) {
+                    throw new IOException("Not a file: " + file);
+                }
+                if (file.length() != length) {
+                    throw new IOException(DIGEST + " collision: " + file);
                 }
-            }
-
-            // Sanity checks on the record file. These should never fail,
-            // but better safe than sorry...
-            if (!file.isFile()) {
-                throw new IOException("Not a file: " + file);
-            }
-            if (file.length() != length) {
-                throw new IOException(DIGEST + " collision: " + file);
             }
 
             return new FileDataRecord(identifier, file);
@@ -269,11 +275,13 @@
     private int deleteOlderRecursive(File file, long min) {
         int count = 0;
         if (file.isFile() && file.exists() && file.canWrite()) {
-            if (file.lastModified() < min) {
-                DataIdentifier id = new DataIdentifier(file.getName());
-                if (!inUse.containsKey(id)) {
-                    file.delete();
-                    count++;
+            synchronized (this) {
+                if (file.lastModified() < min) {
+                    DataIdentifier id = new DataIdentifier(file.getName());
+                    if (!inUse.containsKey(id)) {
+                        file.delete();
+                        count++;
+                    }
                 }
             }
         } else if (file.isDirectory()) {
@@ -283,8 +291,10 @@
             }
             // JCR-1396: FileDataStore Garbage Collector and empty directories
             // Automatic removal of empty directories (but not the root!)
-            if (file != directory && file.list().length == 0) {
-                file.delete();
+            synchronized (this) {
+                if (file != directory && file.list().length == 0) {
+                    file.delete();
+                }
             }
         }
         return count;

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/GarbageCollector.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/GarbageCollector.java?rev=631905&r1=631904&r2=631905&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/GarbageCollector.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/GarbageCollector.java
Thu Feb 28 01:47:51 2008
@@ -23,6 +23,7 @@
 import org.apache.jackrabbit.core.observation.SynchronousEventListener;
 import org.apache.jackrabbit.core.persistence.IterablePersistenceManager;
 import org.apache.jackrabbit.core.state.ItemStateException;
+import org.apache.jackrabbit.core.state.NoSuchItemStateException;
 import org.apache.jackrabbit.core.state.NodeState;
 import org.apache.jackrabbit.core.state.PropertyState;
 import org.apache.jackrabbit.core.value.InternalValue;
@@ -176,19 +177,24 @@
             Iterator it = pm.getAllNodeIds(null, 0);
             while (it.hasNext()) {
                 NodeId id = (NodeId) it.next();
-                NodeState state = pm.load(id);
-                Set propertyNames = state.getPropertyNames();
-                for (Iterator nameIt = propertyNames.iterator(); nameIt
-                        .hasNext();) {
-                    Name name = (Name) nameIt.next();
-                    PropertyId pid = new PropertyId(id, name);
-                    PropertyState ps = pm.load(pid);
-                    if (ps.getType() == PropertyType.BINARY) {
-                        InternalValue[] values = ps.getValues();
-                        for (int j = 0; j < values.length; j++) {
-                            values[j].getBLOBFileValue().getLength();
+                try {
+                    NodeState state = pm.load(id);
+                    Set propertyNames = state.getPropertyNames();
+                    for (Iterator nameIt = propertyNames.iterator(); nameIt
+                            .hasNext();) {
+                        Name name = (Name) nameIt.next();
+                        PropertyId pid = new PropertyId(id, name);
+                        PropertyState ps = pm.load(pid);
+                        if (ps.getType() == PropertyType.BINARY) {
+                            InternalValue[] values = ps.getValues();
+                            for (int j = 0; j < values.length; j++) {
+                                values[j].getBLOBFileValue().getLength();
+                            }
                         }
                     }
+                } catch (NoSuchItemStateException e) {
+                    // the node may have been deleted or moved in the meantime
+                    // ignore it
                 }
             }
         }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java?rev=631905&r1=631904&r2=631905&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/data/db/DbDataStore.java
Thu Feb 28 01:47:51 2008
@@ -40,7 +40,9 @@
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
+import java.util.Map;
 import java.util.Properties;
 import java.util.WeakHashMap;
 
@@ -272,7 +274,7 @@
     /**
      * All data identifiers that are currently in use are in this set until they are garbage
collected.
      */
-    protected WeakHashMap inUse = new WeakHashMap();
+    protected Map inUse = Collections.synchronizedMap(new WeakHashMap());
 
     /**
      * {@inheritDoc}
@@ -396,7 +398,7 @@
     public synchronized int deleteAllOlderThan(long min) throws DataStoreException {
         ConnectionRecoveryManager conn = getConnection();
         try {
-            Iterator it = inUse.keySet().iterator();
+            Iterator it = new ArrayList(inUse.keySet()).iterator();
             while (it.hasNext()) {
                 DataIdentifier identifier = (DataIdentifier) it.next();
                 if (identifier != null) {
@@ -535,20 +537,18 @@
                 return;
             }
         }
-
         Properties prop = new Properties();
         try {
             try {
                 prop.load(in);
             } finally {
-                in.close();
+            in.close();
             }
         } catch (IOException e) {
             String msg = "Configuration error: Could not read properties '" + databaseType
+ ".properties'";
             log.debug(msg);
             throw new DataStoreException(msg);
         }
-
         if (driver == null) {
             driver = getProperty(prop, "driver", driver);
         }

Modified: jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java?rev=631905&r1=631904&r2=631905&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/main/java/org/apache/jackrabbit/core/persistence/bundle/BundleDbPersistenceManager.java
Thu Feb 28 01:47:51 2008
@@ -931,7 +931,7 @@
                 // see also bundleSelectAllIdsFrom SQL statement
                 maxCount += 10;
             }
-            Statement stmt = connectionManager.executeStmt(sql, keys, false, maxCount + 10);
+            Statement stmt = connectionManager.executeStmt(sql, keys, false, maxCount);
             rs = stmt.getResultSet();
             ArrayList result = new ArrayList();
             while ((maxCount == 0 || result.size() < maxCount) && rs.next()) {

Added: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCConcurrentTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCConcurrentTest.java?rev=631905&view=auto
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCConcurrentTest.java
(added)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCConcurrentTest.java
Thu Feb 28 01:47:51 2008
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.jackrabbit.core.data;
+
+import org.apache.jackrabbit.core.RepositoryImpl;
+import org.apache.jackrabbit.test.AbstractJCRTest;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.ByteArrayInputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.util.Random;
+
+import javax.jcr.Node;
+import javax.jcr.Property;
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+
+/**
+ * Test case for concurrent garbage collection
+ */
+public class GCConcurrentTest extends AbstractJCRTest {
+
+    /** logger instance */
+    private static final Logger LOG = LoggerFactory.getLogger(GCConcurrentTest.class);
+
+    public void testGC() throws Exception {
+        Node root = testRootNode;
+        Session session = root.getSession();
+        RepositoryImpl rep = (RepositoryImpl) session.getRepository();
+        if (rep.getDataStore() == null) {
+            LOG.info("testGC skipped. Data store is not used.");
+            return;
+        }
+        GCThread gc = new GCThread(session);
+        Thread gcThread = new Thread(gc, "Datastore Garbage Collector");
+
+        int len = 10 * getTestScale();
+        boolean started = false;
+        for (int i = 0; i < len; i++) {
+            if (!started && i > 5 + len / 100) {
+                started = true;
+                gcThread.start();
+            }
+            Node n = node(root, "test" + i);
+            n.setProperty("data", randomInputStream(i));
+            session.save();
+            LOG.debug("saved: " + i);
+        }
+        Thread.sleep(10);
+        for (int i = 0; i < len; i++) {
+            Node n = root.getNode("test" + i);
+            Property p = n.getProperty("data");
+            InputStream in = p.getStream();
+            InputStream expected = randomInputStream(i);
+            checkStreams(expected, in);
+            n.remove();
+            LOG.debug("removed: " + i);
+            session.save();
+        }
+        Thread.sleep(10);
+        gc.setStop(true);
+        Thread.sleep(10);
+        gc.throwException();
+    }
+
+    private void checkStreams(InputStream expected, InputStream in) throws IOException {
+        while (true) {
+            int e = expected.read();
+            int i = in.read();
+            if (e < 0 || i < 0) {
+                if (e >= 0 || i >= 0) {
+                    fail("expected: " + e + " got: " + i);
+                }
+                break;
+            } else {
+                assertEquals(e, i);
+            }
+        }
+        expected.close();
+        in.close();
+    }
+
+    static InputStream randomInputStream(long seed) {
+        byte[] data = new byte[4096];
+        new Random(seed).nextBytes(data);
+        return new ByteArrayInputStream(data);
+    }
+
+    static Node node(Node n, String x) throws RepositoryException {
+        return n.hasNode(x) ? n.getNode(x) : n.addNode(x);
+    }
+
+    static int getTestScale() {
+        return Integer.parseInt(System.getProperty("jackrabbit.test.scale", "1"));
+    }
+
+}

Propchange: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCConcurrentTest.java
------------------------------------------------------------------------------
    svn:eol-style = native

Added: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCThread.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCThread.java?rev=631905&view=auto
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCThread.java
(added)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCThread.java
Thu Feb 28 01:47:51 2008
@@ -0,0 +1,103 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You 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 org.apache.jackrabbit.core.data;
+
+import org.apache.jackrabbit.core.SessionImpl;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.util.Iterator;
+
+import javax.jcr.Node;
+import javax.jcr.RepositoryException;
+import javax.jcr.Session;
+
+/**
+ * Helper class that runs data store garbage collection as a background thread.
+ */
+public class GCThread implements Runnable, ScanEventListener {
+
+    /** logger instance */
+    private static final Logger LOG = LoggerFactory.getLogger(GCThread.class);
+
+    private boolean stop;
+    private Session session;
+    private Exception exception;
+
+    public GCThread(Session session) {
+        this.session = session;
+    }
+
+    public void run() {
+
+        try {
+            GarbageCollector gc = ((SessionImpl) session).createDataStoreGarbageCollector();
+            gc.setScanEventListener(this);
+            while (!stop) {
+                LOG.debug("Scanning...");
+                gc.scan();
+                int count = listIdentifiers(gc);
+                LOG.debug("Stop; currently " + count + " identifiers");
+                gc.stopScan();
+                int numDeleted = gc.deleteUnused();
+                if (numDeleted > 0) {
+                    LOG.debug("Deleted " + numDeleted + " identifiers");
+                }
+                LOG.debug("Waiting...");
+                Thread.sleep(10);
+            }
+        } catch (Exception ex) {
+            LOG.error("Error scanning", ex);
+            exception = ex;
+        }
+    }
+
+    public void setStop(boolean stop) {
+        this.stop = stop;
+    }
+
+    public Exception getException() {
+        return exception;
+    }
+
+    private int listIdentifiers(GarbageCollector gc) throws DataStoreException {
+        Iterator it = gc.getDataStore().getAllIdentifiers();
+        int count = 0;
+        while (it.hasNext()) {
+            DataIdentifier id = (DataIdentifier) it.next();
+            LOG.debug("  " + id);
+            count++;
+        }
+        return count;
+    }
+
+    public void throwException() throws Exception {
+        if (exception != null) {
+            throw exception;
+        }
+    }
+
+    public void afterScanning(Node n) throws RepositoryException {
+    }
+
+    public void beforeScanning(Node n) throws RepositoryException {
+    }
+
+    public void done() {
+    }
+
+}

Propchange: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/GCThread.java
------------------------------------------------------------------------------
    svn:eol-style = native

Modified: jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/TestAll.java
URL: http://svn.apache.org/viewvc/jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/TestAll.java?rev=631905&r1=631904&r2=631905&view=diff
==============================================================================
--- jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/TestAll.java
(original)
+++ jackrabbit/trunk/jackrabbit-core/src/test/java/org/apache/jackrabbit/core/data/TestAll.java
Thu Feb 28 01:47:51 2008
@@ -35,6 +35,7 @@
         suite.addTestSuite(NodeTypeTest.class);
         suite.addTestSuite(ExportImportTest.class);
         suite.addTestSuite(GarbageCollectorTest.class);
+        suite.addTestSuite(GCConcurrentTest.class);
         suite.addTestSuite(PersistenceManagerIteratorTest.class);
         suite.addTestSuite(CopyValueTest.class);
         return suite;



Mime
View raw message