GEODE-2240: prevent unsafe concurrent access to ArrayList Fixed deadlock by adding expiredTombstonesLock.
Project: http://git-wip-us.apache.org/repos/asf/geode/repo Commit: http://git-wip-us.apache.org/repos/asf/geode/commit/4d587789 Tree: http://git-wip-us.apache.org/repos/asf/geode/tree/4d587789 Diff: http://git-wip-us.apache.org/repos/asf/geode/diff/4d587789 Branch: refs/heads/master Commit: 4d58778985b832a7899056a06fb0de0f2995f55e Parents: 85d45e8 Author: Darrel Schneider <dschnei...@pivotal.io> Authored: Tue Jan 3 17:00:07 2017 -0800 Committer: Darrel Schneider <dschnei...@pivotal.io> Committed: Fri Jan 20 14:15:34 2017 -0800 ---------------------------------------------------------------------- .../geode/internal/cache/TombstoneService.java | 22 ++++++++++++-------- 1 file changed, 13 insertions(+), 9 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/geode/blob/4d587789/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java ---------------------------------------------------------------------- diff --git a/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java b/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java index 2840134..ca682bc 100644 --- a/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java +++ b/geode-core/src/main/java/org/apache/geode/internal/cache/TombstoneService.java @@ -406,10 +406,10 @@ public class TombstoneService { */ private final ExecutorService executor; /** - * tombstones that have expired and are awaiting batch removal. This variable is only accessed - * by the sweeper thread and so is not guarded + * tombstones that have expired and are awaiting batch removal. */ private final List<Tombstone> expiredTombstones; + private final Object expiredTombstonesLock = new Object(); /** * Force batch expiration @@ -470,7 +470,7 @@ public class TombstoneService { protected boolean removeExpiredIf(Predicate<Tombstone> predicate) { boolean result = false; long removalSize = 0; - synchronized (getBlockGCLock()) { + synchronized (expiredTombstonesLock) { // Iterate in reverse order to optimize lots of removes. // Since expiredTombstones is an ArrayList removing from // low indexes requires moving everything at a higher index down. @@ -520,11 +520,13 @@ public class TombstoneService { // Update the GC RVV for all of the affected regions. // We need to do this so that we can persist the GC RVV before // we start removing entries from the map. - for (Tombstone t : expiredTombstones) { - DistributedRegion tr = (DistributedRegion) t.region; - tr.getVersionVector().recordGCVersion(t.getMemberID(), t.getRegionVersion()); - if (!reapedKeys.containsKey(tr)) { - reapedKeys.put(tr, Collections.emptySet()); + synchronized (expiredTombstonesLock) { + for (Tombstone t : expiredTombstones) { + DistributedRegion tr = (DistributedRegion) t.region; + tr.getVersionVector().recordGCVersion(t.getMemberID(), t.getRegionVersion()); + if (!reapedKeys.containsKey(tr)) { + reapedKeys.put(tr, Collections.emptySet()); + } } } @@ -669,7 +671,9 @@ public class TombstoneService { if (logger.isTraceEnabled(LogMarker.TOMBSTONE)) { logger.trace(LogMarker.TOMBSTONE, "adding expired tombstone {} to batch", tombstone); } - expiredTombstones.add(tombstone); + synchronized (expiredTombstonesLock) { + expiredTombstones.add(tombstone); + } } @Override
