You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@jackrabbit.apache.org by th...@apache.org on 2008/02/28 10:47:53 UTC
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/
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;