Repository: incubator-geode
Updated Branches:
  refs/heads/develop f1df6fc59 -> a3bd25664


GEODE-706 Fixed race condition between expiry thread and put thread.

There was possibility that expiry thread destroying the entry and
another thread doing update on same key. In this case expiry thread
cancel's existing task and update thread adds expiry task. But this
tasks are refer by regionEntry, which is same for both the threads.
So in this case if expiry thread cancel's task after update thread
then that entry will never expire.


Project: http://git-wip-us.apache.org/repos/asf/incubator-geode/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-geode/commit/a3bd2566
Tree: http://git-wip-us.apache.org/repos/asf/incubator-geode/tree/a3bd2566
Diff: http://git-wip-us.apache.org/repos/asf/incubator-geode/diff/a3bd2566

Branch: refs/heads/develop
Commit: a3bd256648e30b0bf04c565a8f21d00868c29806
Parents: f1df6fc
Author: Hitesh Khamesra <hkhame...@pivotal.io>
Authored: Fri Oct 14 14:00:25 2016 -0700
Committer: Hitesh Khamesra <hkhame...@pivotal.io>
Committed: Mon Oct 17 14:30:46 2016 -0700

----------------------------------------------------------------------
 .../geode/internal/cache/AbstractRegionMap.java  |  4 ++--
 .../geode/internal/cache/EntryEventImpl.java     |  9 +++++++++
 .../geode/internal/cache/EntryExpiryTask.java    |  3 ++-
 .../apache/geode/internal/cache/LocalRegion.java | 19 +++++++++++++++----
 4 files changed, 28 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/a3bd2566/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
 
b/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
index 5861e9a..e02c7e1 100644
--- 
a/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/cache/AbstractRegionMap.java
@@ -1513,9 +1513,9 @@ public abstract class AbstractRegionMap implements 
RegionMap {
           } finally {
             if (opCompleted) {
               if (re != null) {
-                owner.cancelExpiryTask(re);
+                owner.cancelExpiryTask(re, event.getExpiryTask());
               } else if (tombstone != null) {
-                owner.cancelExpiryTask(tombstone);
+                owner.cancelExpiryTask(tombstone, event.getExpiryTask());
               }
             }
           }

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/a3bd2566/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java 
b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
index 6a964c0..d059aab 100644
--- 
a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryEventImpl.java
@@ -73,6 +73,7 @@ public class EntryEventImpl
   // PACKAGE FIELDS //
   public transient LocalRegion region;
   private transient RegionEntry re;
+  private transient ExpiryTask expiryTask;
 
   protected KeyInfo keyInfo;
 
@@ -2853,4 +2854,12 @@ public class EntryEventImpl
   public boolean isOldValueOffHeap() {
     return isOffHeapReference(this.oldValue);
   }
+
+  public ExpiryTask getExpiryTask() {
+    return expiryTask;
+  }
+
+  public void setExpiryTask(ExpiryTask expiryTask) {
+    this.expiryTask = expiryTask;
+  }
 }
\ No newline at end of file

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/a3bd2566/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java 
b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
index 816f32f..0c20d32 100644
--- 
a/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
+++ 
b/geode-core/src/main/java/org/apache/geode/internal/cache/EntryExpiryTask.java
@@ -117,6 +117,7 @@ public class EntryExpiryTask extends ExpiryTask {
     @Released EntryEventImpl event = EntryEventImpl.create(
         lr, Operation.EXPIRE_DESTROY, key, null,
         createExpireEntryCallback(lr, key), false, lr.getMyId());
+    event.setExpiryTask(this);
     try {
     event.setPendingSecondaryExpireDestroy(isPending);
     if (lr.generateEventID()) {
@@ -229,7 +230,7 @@ public class EntryExpiryTask extends ExpiryTask {
     // so the next call to addExpiryTaskIfAbsent will
     // add a new task instead of doing nothing, which would
     // erroneously cancel expiration for this key.
-    getLocalRegion().cancelExpiryTask(this.re);
+    getLocalRegion().cancelExpiryTask(this.re, null);
     getLocalRegion().performExpiryTimeout(this);
   }
   

http://git-wip-us.apache.org/repos/asf/incubator-geode/blob/a3bd2566/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
----------------------------------------------------------------------
diff --git 
a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java 
b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
index a6951de..ac4c705 100644
--- a/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
+++ b/geode-core/src/main/java/org/apache/geode/internal/cache/LocalRegion.java
@@ -8717,14 +8717,25 @@ public class LocalRegion extends AbstractRegion
       }
     }
   }
+  
+  void cancelExpiryTask(RegionEntry re) {
+    cancelExpiryTask(re, null);
+  }
 
-  void cancelExpiryTask(RegionEntry re)
+  void cancelExpiryTask(RegionEntry re, ExpiryTask expiryTask)
   {
-    EntryExpiryTask oldTask = this.entryExpiryTasks.remove(re);
-    if (oldTask != null) {
-      if (oldTask.cancel()) {
+    if (expiryTask != null) {
+      this.entryExpiryTasks.remove(re, expiryTask);
+      if (expiryTask.cancel()) {
         this.cache.getExpirationScheduler().incCancels();
       }
+    } else {
+      EntryExpiryTask oldTask = this.entryExpiryTasks.remove(re);
+      if (oldTask != null) {
+        if (oldTask.cancel()) {
+          this.cache.getExpirationScheduler().incCancels();
+        }
+      }
     }
   }
 

Reply via email to