You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-commits@jackrabbit.apache.org by mr...@apache.org on 2015/06/10 10:36:02 UTC
svn commit: r1684601 - in /jackrabbit/oak/trunk/oak-core/src:
main/java/org/apache/jackrabbit/oak/plugins/document/
test/java/org/apache/jackrabbit/oak/plugins/document/
Author: mreutegg
Date: Wed Jun 10 08:36:01 2015
New Revision: 1684601
URL: http://svn.apache.org/r1684601
Log:
OAK-2620: Release merge lock before branch is reset
Modified:
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBranch.java
jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRootBuilder.java
jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBranch.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBranch.java?rev=1684601&r1=1684600&r2=1684601&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBranch.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreBranch.java Wed Jun 10 08:36:01 2015
@@ -191,6 +191,14 @@ class DocumentNodeStoreBranch implements
//------------------------------< internal >--------------------------------
+ /**
+ * For test purposes only!
+ */
+ @Nonnull
+ ReadWriteLock getMergeLock() {
+ return mergeLock;
+ }
+
@Nonnull
private NodeState merge0(@Nonnull CommitHook hook,
@Nonnull CommitInfo info,
@@ -212,28 +220,17 @@ class DocumentNodeStoreBranch implements
}
}
try {
- final long start = perfLogger.start();
- Lock lock = acquireMergeLock(exclusive);
- try {
- perfLogger.end(start, 1, "Merge - Acquired lock");
- return branchState.merge(checkNotNull(hook), checkNotNull(info));
- } catch (CommitFailedException e) {
- LOG.trace("Merge Error", e);
- ex = e;
- // only retry on merge failures. these may be caused by
- // changes introduce by a commit hook and may be resolved
- // by a rebase and running the hook again
- if (!e.isOfType(MERGE)) {
- throw e;
- }
- } finally {
- if (lock != null) {
- lock.unlock();
- }
+ return branchState.merge(checkNotNull(hook),
+ checkNotNull(info), exclusive);
+ } catch (CommitFailedException e) {
+ LOG.trace("Merge Error", e);
+ ex = e;
+ // only retry on merge failures. these may be caused by
+ // changes introduce by a commit hook and may be resolved
+ // by a rebase and running the hook again
+ if (!e.isOfType(MERGE)) {
+ throw e;
}
- } catch (InterruptedException e) {
- throw new CommitFailedException(OAK, 1,
- "Unable to acquire merge lock", e);
}
}
// if we get here retrying failed
@@ -249,21 +246,30 @@ class DocumentNodeStoreBranch implements
* @param exclusive whether to acquire the merge lock exclusive.
* @return the acquired merge lock or {@code null} if the operation timed
* out.
- * @throws InterruptedException if the current thread is interrupted while
- * acquiring the lock
+ * @throws CommitFailedException if the current thread is interrupted while
+ * acquiring the lock
*/
@CheckForNull
private Lock acquireMergeLock(boolean exclusive)
- throws InterruptedException {
+ throws CommitFailedException {
+ final long start = perfLogger.start();
Lock lock;
if (exclusive) {
lock = mergeLock.writeLock();
} else {
lock = mergeLock.readLock();
}
- boolean acquired = lock.tryLock(maxLockTryTimeMS, MILLISECONDS);
- if (!acquired) {
- String mode = exclusive ? "exclusive" : "shared";
+ boolean acquired;
+ try {
+ acquired = lock.tryLock(maxLockTryTimeMS, MILLISECONDS);
+ } catch (InterruptedException e) {
+ throw new CommitFailedException(OAK, 1,
+ "Unable to acquire merge lock", e);
+ }
+ String mode = exclusive ? "exclusive" : "shared";
+ if (acquired) {
+ perfLogger.end(start, 1, "Merge - Acquired lock ({})", mode);
+ } else {
LOG.info("Time out while acquiring merge lock ({})", mode);
lock = null;
}
@@ -409,6 +415,8 @@ class DocumentNodeStoreBranch implements
*
* @param hook the commit hook to run.
* @param info the associated commit info.
+ * @param exclusive whether the merge lock must be acquired exclusively
+ * or shared while performing the merge.
* @return the result of the merge.
* @throws CommitFailedException if a commit hook rejected the changes
* or the actual merge operation failed. An implementation must
@@ -416,8 +424,9 @@ class DocumentNodeStoreBranch implements
* indicate the cause of the exception.
*/
@Nonnull
- abstract NodeState merge(
- @Nonnull CommitHook hook, @Nonnull CommitInfo info)
+ abstract NodeState merge(@Nonnull CommitHook hook,
+ @Nonnull CommitInfo info,
+ boolean exclusive)
throws CommitFailedException;
}
@@ -428,7 +437,7 @@ class DocumentNodeStoreBranch implements
* <ul>
* <li>{@link InMemory} on {@link #setRoot(NodeState)} if the new root differs
* from the current base</li>.
- * <li>{@link Merged} on {@link #merge(CommitHook, CommitInfo)}</li>
+ * <li>{@link Merged} on {@link BranchState#merge(CommitHook, CommitInfo, boolean)}</li>
* </ul>
*/
private class Unmodified extends BranchState {
@@ -461,7 +470,9 @@ class DocumentNodeStoreBranch implements
@Override
@Nonnull
- NodeState merge(@Nonnull CommitHook hook, @Nonnull CommitInfo info) {
+ NodeState merge(@Nonnull CommitHook hook,
+ @Nonnull CommitInfo info,
+ boolean exclusive) {
branchState = new Merged(base);
return base;
}
@@ -476,7 +487,7 @@ class DocumentNodeStoreBranch implements
* <li>{@link Unmodified} on {@link #setRoot(NodeState)} if the new root is the same
* as the base of this branch or
* <li>{@link Persisted} otherwise.
- * <li>{@link Merged} on {@link #merge(CommitHook, CommitInfo)}</li>
+ * <li>{@link Merged} on {@link BranchState#merge(CommitHook, CommitInfo, boolean)}</li>
* </ul>
*/
private class InMemory extends BranchState {
@@ -520,20 +531,31 @@ class DocumentNodeStoreBranch implements
@Override
@Nonnull
- NodeState merge(@Nonnull CommitHook hook, @Nonnull CommitInfo info)
+ NodeState merge(@Nonnull CommitHook hook,
+ @Nonnull CommitInfo info,
+ boolean exclusive)
throws CommitFailedException {
checkNotNull(hook);
checkNotNull(info);
- rebase();
- NodeState toCommit = hook.processCommit(base, head, info);
+ Lock lock = acquireMergeLock(exclusive);
try {
- NodeState newHead = DocumentNodeStoreBranch.this.persist(toCommit, base, info);
- branchState = new Merged(base);
- return newHead;
- } catch(DocumentStoreException e) {
- throw new CommitFailedException(MERGE, 1, "Failed to merge changes to the underlying store", e);
- } catch (Exception e) {
- throw new CommitFailedException(OAK, 1, "Failed to merge changes to the underlying store", e);
+ rebase();
+ NodeState toCommit = hook.processCommit(base, head, info);
+ try {
+ NodeState newHead = DocumentNodeStoreBranch.this.persist(toCommit, base, info);
+ branchState = new Merged(base);
+ return newHead;
+ } catch(DocumentStoreException e) {
+ throw new CommitFailedException(MERGE, 1,
+ "Failed to merge changes to the underlying store", e);
+ } catch (Exception e) {
+ throw new CommitFailedException(OAK, 1,
+ "Failed to merge changes to the underlying store", e);
+ }
+ } finally {
+ if (lock != null) {
+ lock.unlock();
+ }
}
}
}
@@ -544,8 +566,8 @@ class DocumentNodeStoreBranch implements
* <p>
* Transitions to:
* <ul>
- * <li>{@link ResetFailed} on failed reset in {@link #merge(CommitHook, CommitInfo)}</li>
- * <li>{@link Merged} on successful {@link #merge(CommitHook, CommitInfo)}</li>
+ * <li>{@link ResetFailed} on failed reset in {@link BranchState#merge(CommitHook, CommitInfo, boolean)}</li>
+ * <li>{@link Merged} on successful {@link BranchState#merge(CommitHook, CommitInfo, boolean)}</li>
* </ul>
*/
private class Persisted extends BranchState {
@@ -618,10 +640,12 @@ class DocumentNodeStoreBranch implements
@Override
@Nonnull
NodeState merge(@Nonnull final CommitHook hook,
- @Nonnull final CommitInfo info)
+ @Nonnull final CommitInfo info,
+ boolean exclusive)
throws CommitFailedException {
boolean success = false;
DocumentNodeState previousHead = head;
+ Lock lock = acquireMergeLock(exclusive);
try {
rebase();
previousHead = head;
@@ -642,6 +666,9 @@ class DocumentNodeStoreBranch implements
throw new CommitFailedException(MERGE, 1,
"Failed to merge changes to the underlying store", e);
} finally {
+ if (lock != null) {
+ lock.unlock();
+ }
if (!success) {
resetBranch(head, previousHead);
}
@@ -700,7 +727,9 @@ class DocumentNodeStoreBranch implements
@Override
@Nonnull
- NodeState merge(@Nonnull CommitHook hook, @Nonnull CommitInfo info) {
+ NodeState merge(@Nonnull CommitHook hook,
+ @Nonnull CommitInfo info,
+ boolean exclusive) {
throw new IllegalStateException("Branch has already been merged");
}
}
@@ -747,7 +776,9 @@ class DocumentNodeStoreBranch implements
*/
@Nonnull
@Override
- NodeState merge(@Nonnull CommitHook hook, @Nonnull CommitInfo info)
+ NodeState merge(@Nonnull CommitHook hook,
+ @Nonnull CommitInfo info,
+ boolean exclusive)
throws CommitFailedException {
throw ex;
}
Modified: jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRootBuilder.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRootBuilder.java?rev=1684601&r1=1684600&r2=1684601&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRootBuilder.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/document/DocumentRootBuilder.java Wed Jun 10 08:36:01 2015
@@ -95,7 +95,7 @@ class DocumentRootBuilder extends Abstra
@Override
protected void updated() {
- if (updates++ > UPDATE_LIMIT) {
+ if (++updates > UPDATE_LIMIT) {
purge();
}
}
Modified: jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java
URL: http://svn.apache.org/viewvc/jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java?rev=1684601&r1=1684600&r2=1684601&view=diff
==============================================================================
--- jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java (original)
+++ jackrabbit/oak/trunk/oak-core/src/test/java/org/apache/jackrabbit/oak/plugins/document/DocumentNodeStoreTest.java Wed Jun 10 08:36:01 2015
@@ -50,6 +50,8 @@ import java.util.concurrent.CountDownLat
import java.util.concurrent.Semaphore;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;
@@ -64,6 +66,8 @@ import org.apache.jackrabbit.oak.api.Typ
import org.apache.jackrabbit.oak.plugins.commit.AnnotatingConflictHandler;
import org.apache.jackrabbit.oak.plugins.commit.ConflictHook;
import org.apache.jackrabbit.oak.plugins.commit.ConflictValidatorProvider;
+import org.apache.jackrabbit.oak.plugins.document.UpdateOp.Key;
+import org.apache.jackrabbit.oak.plugins.document.UpdateOp.Operation;
import org.apache.jackrabbit.oak.plugins.document.cache.CacheInvalidationStats;
import org.apache.jackrabbit.oak.plugins.document.memory.MemoryDocumentStore;
import org.apache.jackrabbit.oak.plugins.document.util.TimingDocumentStoreWrapper;
@@ -1815,6 +1819,75 @@ public class DocumentNodeStoreTest {
ns.dispose();
}
+ // OAK-2620
+ @Test
+ public void nonBlockingReset() throws Exception {
+ final List<String> failure = Lists.newArrayList();
+ final AtomicReference<ReentrantReadWriteLock> mergeLock
+ = new AtomicReference<ReentrantReadWriteLock>();
+ MemoryDocumentStore store = new MemoryDocumentStore() {
+ @Override
+ public <T extends Document> T findAndUpdate(Collection<T> collection,
+ UpdateOp update) {
+ for (Map.Entry<Key, Operation> entry : update.getChanges().entrySet()) {
+ if (entry.getKey().getName().equals(NodeDocument.COLLISIONS)) {
+ ReentrantReadWriteLock rwLock = mergeLock.get();
+ if (rwLock.getReadHoldCount() > 0
+ || rwLock.getWriteHoldCount() > 0) {
+ failure.add("Branch reset still holds merge lock");
+ break;
+ }
+ }
+ }
+ return super.findAndUpdate(collection, update);
+ }
+ };
+ DocumentNodeStore ds = new DocumentMK.Builder()
+ .setDocumentStore(store)
+ .setAsyncDelay(0).getNodeStore();
+ ds.setMaxBackOffMillis(0); // do not retry merges
+
+ DocumentNodeState root = ds.getRoot();
+ final DocumentNodeStoreBranch b = ds.createBranch(root);
+ // branch state is now Unmodified
+
+ assertTrue(b.getMergeLock() instanceof ReentrantReadWriteLock);
+ mergeLock.set((ReentrantReadWriteLock) b.getMergeLock());
+
+ NodeBuilder builder = root.builder();
+ builder.child("foo");
+ b.setRoot(builder.getNodeState());
+ // branch state is now InMemory
+ builder.child("bar");
+ b.setRoot(builder.getNodeState());
+ // branch state is now Persisted
+
+ try {
+ b.merge(new CommitHook() {
+ @Nonnull
+ @Override
+ public NodeState processCommit(NodeState before,
+ NodeState after,
+ CommitInfo info)
+ throws CommitFailedException {
+ NodeBuilder foo = after.builder().child("foo");
+ for (int i = 0; i <= DocumentRootBuilder.UPDATE_LIMIT; i++) {
+ foo.setProperty("prop", i);
+ }
+ throw new CommitFailedException("Fail", 0, "");
+ }
+ }, CommitInfo.EMPTY);
+ } catch (CommitFailedException e) {
+ // expected
+ }
+
+ ds.dispose();
+
+ for (String s : failure) {
+ fail(s);
+ }
+ }
+
private static DocumentNodeState asDocumentNodeState(NodeState state) {
if (!(state instanceof DocumentNodeState)) {
throw new IllegalArgumentException("Not a DocumentNodeState");