Never block on key in `LiveVersionMap#pruneTombstones` #28736

s1monw · 2018-02-19T21:03:22Z

Pruning tombstones is best effort and should not block if a key is currently
locked. This can cause a deadlock in rare situations if we switch of append
only optimization while heavily updating the same key in the engine
while the LiveVersionMap is locked. This is very rare since this code
patch only executed every 15 seconds by default since that is the interval
we try to prune the deletes in the version map.

Closes #28714

Pruning tombstones is best effort and should not block if a key is currently locked. This can cause a deadlock in rare situations if we switch of append only optimization while heavily updating the same key in the engine while the LiveVersionMap is locked. This is very rare since this code patch only executed every 15 seconds by default since that is the interval we try to prune the deletes in the version map. Closes elastic#28714

danielmitterdorfer

Looks fine overall. I left a few comments and suggestions.

danielmitterdorfer · 2018-02-20T08:01:28Z

server/src/main/java/org/elasticsearch/common/util/concurrent/KeyedLock.java

+        }
+        if (perNodeLock.tryLock()) { // ok we got it - make sure we increment it accordingly otherwise release it again
+            int i;
+            while ((i = perNodeLock.count.get()) > 0) {


Is this necessary? Shouldn't we be able to return the ReleasableLock once we have acquired perNodeLock?

I left a comment regarding this

danielmitterdorfer · 2018-02-20T08:02:20Z

server/src/main/java/org/elasticsearch/common/util/concurrent/KeyedLock.java

        if (decrementAndGet == 0) {
            map.remove(key, lock);
        }
+        assert decrementAndGet >= 0 : decrementAndGet + " must be >= 0 but wasn't";


danielmitterdorfer · 2018-02-20T08:10:22Z

server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java

-                            removeTombstoneUnderLock(uid);
+            Releasable lock = keyedLock.tryAcquire(uid);
+            if (lock != null) {
+                try (Releasable ignored = lock) { // can we do it without this lock on each value? maybe batch to a set and get


Does it make sense to call tryAcquire in the try-with-resources block and do the null check in the block? i.e.:

try (Releasable lock = keyedLock.tryAcquire(uid)) { if (lock != null) { DeleteVersionValue versionValue = tombstones.get(uid); // ... } }

That implementation would not need a dummy ignored variable and instead actually use the returned value (i.e. lock).

it does I didn't know you can do that with a null value.

danielmitterdorfer · 2018-02-20T08:11:18Z

server/src/test/java/org/elasticsearch/common/util/concurrent/KeyedLockTests.java

@@ -125,6 +153,7 @@ public AcquireAndReleaseThread(CountDownLatch startLatch, KeyedLock<String> conn
            this.names = names;
            this.counter = counter;
            this.safeCounter = safeCounter;
+


Nit: Empty line

danielmitterdorfer · 2018-02-20T08:16:39Z

server/src/main/java/org/elasticsearch/index/engine/LiveVersionMap.java

-                        final boolean isNotTrackedByCurrentMaps = versionValue.time < maps.getMinDeleteTimestamp();
-                        if (isNotTrackedByCurrentMaps) {
-                            removeTombstoneUnderLock(uid);
+            Releasable lock = keyedLock.tryAcquire(uid);


I have to admit that it took me a moment to understand why this is avoiding the deadlock as there need to be two locks involved that are acquired by two threads in opposite order. The reason why this fix works is that this is the inner of the two involved locks. Then there are two cases:

We get this lock, remove the tombstone and return the lock. If another thread is trying to acquire this lock, it can do so after we have left the try-with-resources block.

We do not get the lock. Then we will not wait but rather give up (this time).

(just wrote this down for my own reference.)

s1monw · 2018-02-20T08:53:12Z

@danielmitterdorfer I pushed some changes to make things more clear.

danielmitterdorfer

Thanks @s1monw. LGTM!

s1monw · 2018-02-20T10:09:48Z

I will merge this once the build is green

s1monw · 2018-02-20T11:32:59Z

@ywelsch @danielmitterdorfer I added a new test that reproduces the issue on the engine level and fails 100% of the time without the fix.

danielmitterdorfer · 2018-02-20T11:42:51Z

server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

+                // holding the lock that pruneTombstones needs and we have a deadlock
+                CountDownLatch awaitStared = new CountDownLatch(1);
+                Thread thread = new Thread(() -> {
+                    awaitStared.countDown();


Nit: awaitStarted

danielmitterdorfer · 2018-02-20T11:43:24Z

server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

+                engine.index(new Engine.Index(newUid(document2), document2, SequenceNumbers.UNASSIGNED_SEQ_NO, 0,
+                    Versions.MATCH_ANY, VersionType.INTERNAL, Engine.Operation.Origin.PRIMARY, System.nanoTime(), 0, false));
+                engine.refresh("test");
+                ParsedDocument dummyDocument = testParsedDocument(Integer.toString(3), null, testDocumentWithTextField(), SOURCE, null);


Nit: should be named document3 for consistency?

danielmitterdorfer · 2018-02-20T11:44:18Z

Thanks for the test case @s1monw! I left two very minor comments but otherwise the test looks fine to me.

s1monw · 2018-02-20T15:54:11Z

@bleskes ping just FYI

Pruning tombstones is best effort and should not block if a key is currently locked. This can cause a deadlock in rare situations if we switch of append only optimization while heavily updating the same key in the engine while the LiveVersionMap is locked. This is very rare since this code patch only executed every 15 seconds by default since that is the interval we try to prune the deletes in the version map. Closes #28714

bleskes · 2018-02-28T19:31:13Z

@s1monw thanks for the ping. Good catch.

Pruning tombstones is best effort and should not block if a key is currently locked. This can cause a deadlock in rare situations if we switch of append only optimization while heavily updating the same key in the engine while the LiveVersionMap is locked. This is very rare since this code patch only executed every 15 seconds by default since that is the interval we try to prune the deletes in the version map. Closes elastic#28714

s1monw added >bug v7.0.0 v6.3.0 v6.2.3 :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. labels Feb 19, 2018

s1monw requested review from bleskes, danielmitterdorfer and ywelsch February 19, 2018 21:03

danielmitterdorfer reviewed Feb 20, 2018

View reviewed changes

s1monw added 2 commits February 20, 2018 09:36

Merge branch 'master' into issues/28714

f86962d

apply feedback from @danielmitterdorfer

0e847ec

danielmitterdorfer approved these changes Feb 20, 2018

View reviewed changes

add engine test to reproduce the issue

42d785c

danielmitterdorfer reviewed Feb 20, 2018

View reviewed changes

apply comments

3edb7af

s1monw merged commit b008706 into elastic:master Feb 20, 2018

s1monw deleted the issues/28714 branch February 20, 2018 15:35

s1monw added the backport pending label Feb 20, 2018

s1monw removed the backport pending label Feb 20, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Never block on key in `LiveVersionMap#pruneTombstones` #28736

Never block on key in `LiveVersionMap#pruneTombstones` #28736

s1monw commented Feb 19, 2018 •

edited

Loading

danielmitterdorfer left a comment

danielmitterdorfer Feb 20, 2018

s1monw Feb 20, 2018

danielmitterdorfer Feb 20, 2018

danielmitterdorfer Feb 20, 2018

s1monw Feb 20, 2018

danielmitterdorfer Feb 20, 2018

danielmitterdorfer Feb 20, 2018

s1monw Feb 20, 2018

s1monw commented Feb 20, 2018

danielmitterdorfer left a comment

s1monw commented Feb 20, 2018

s1monw commented Feb 20, 2018

danielmitterdorfer Feb 20, 2018

danielmitterdorfer Feb 20, 2018

danielmitterdorfer commented Feb 20, 2018

s1monw commented Feb 20, 2018

bleskes commented Feb 28, 2018

Never block on key in LiveVersionMap#pruneTombstones #28736

Never block on key in LiveVersionMap#pruneTombstones #28736

Conversation

s1monw commented Feb 19, 2018 • edited Loading

danielmitterdorfer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

s1monw commented Feb 20, 2018

danielmitterdorfer left a comment

Choose a reason for hiding this comment

s1monw commented Feb 20, 2018

s1monw commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielmitterdorfer commented Feb 20, 2018

s1monw commented Feb 20, 2018

bleskes commented Feb 28, 2018

Never block on key in `LiveVersionMap#pruneTombstones` #28736

Never block on key in `LiveVersionMap#pruneTombstones` #28736

s1monw commented Feb 19, 2018 •

edited

Loading