Merge pull request #200 from basho/mv-iter-valid2

Cache Valid() state of iterator
basho · Jun 21, 2016 · 9c28214 · 9c28214
2 parents be98f16 + a7c191c
commit 9c28214
Show file tree

Hide file tree

Showing 4 changed files with 30 additions and 9 deletions.
diff --git a/BASHO_RELEASES b/BASHO_RELEASES
@@ -1,3 +1,9 @@
+github.com tag 2.0.22 - June 21, 2016
+-------------------------------------
+ - branch mv-iter-valid (PR 199): adjust recent code accessing iterator->Valid() where
+   underlying iterator could be deleted ... now cache Valid().
+ - eleveldb only, no leveldb changes
+
 github.com tag 2.0.21 - June 17, 2016
 -------------------------------------
  - update to leveldb 2.0.21 (correct hot threads for iterator hang)

diff --git a/c_src/refobjects.cc b/c_src/refobjects.cc
@@ -393,7 +393,7 @@ LevelIteratorWrapper::LevelIteratorWrapper(
       m_HandoffAtomic(0), m_KeysOnly(KeysOnly), m_PrefetchStarted(false),
       m_Options(Options), itr_ref(itr_ref),
       m_IteratorStale(0), m_StillUse(true),
-      m_IteratorCreated(0), m_LastLogReport(0), m_MoveCount(0)
+      m_IteratorCreated(0), m_LastLogReport(0), m_MoveCount(0), m_IsValid(false)
 {
     struct timeval tv;
 

diff --git a/c_src/refobjects.h b/c_src/refobjects.h
@@ -131,8 +131,10 @@ class ReferencePtr
 
     ~ReferencePtr()
     {
-        if (NULL!=t)
-            t->RefDec();
+        TargetT * temp_ptr(t);
+        t=NULL;
+        if (NULL!=temp_ptr)
+            temp_ptr->RefDec();
     }
 
     void assign(TargetT * _t)
@@ -233,6 +235,9 @@ class LevelIteratorWrapper : public RefObject
     time_t m_LastLogReport;                   //!< LOG message was last written
     size_t m_MoveCount;                       //!< number of calls to MoveItem
 
+    // read by Erlang thread, maintained by eleveldb MoveItem::DoWork
+    volatile bool m_IsValid;                  //!< iterator state after last operation
+
     LevelIteratorWrapper(ItrObject * ItrPtr, bool KeysOnly,
                          leveldb::ReadOptions & Options, ERL_NIF_TERM itr_ref);
 
@@ -242,9 +247,12 @@ class LevelIteratorWrapper : public RefObject
     }   // ~LevelIteratorWrapper
 
     leveldb::Iterator * get() {return(m_Iterator);};
-    leveldb::Iterator * operator->() {return(m_Iterator);};
 
-    bool Valid() {return(NULL!=m_Iterator && m_Iterator->Valid());};
+    volatile bool Valid() {return(m_IsValid);};
+    void SetValid(bool flag) {m_IsValid=flag;};
+
+    // warning, only valid with Valid() otherwise potential
+    //   segfault if iterator purged to fix AAE'ism
     leveldb::Slice key() {return(m_Iterator->key());};
     leveldb::Slice value() {return(m_Iterator->value());};
 
@@ -253,15 +261,19 @@ class LevelIteratorWrapper : public RefObject
     {
         if (NULL!=m_Snapshot)
         {
-            // leveldb performs actual "delete" call on m_Shapshot's pointer
-            m_DbPtr->m_Db->ReleaseSnapshot(m_Snapshot);
+            const leveldb::Snapshot * temp_snap(m_Snapshot);
+
             m_Snapshot=NULL;
+            // leveldb performs actual "delete" call on m_Shapshot's pointer
+            m_DbPtr->m_Db->ReleaseSnapshot(temp_snap);
         }   // if
 
         if (NULL!=m_Iterator)
         {
-            delete m_Iterator;
+            leveldb::Iterator * temp_iter(m_Iterator);
+
             m_Iterator=NULL;
+            delete temp_iter;
         }   // if
     }   // PurgeIterator
 

diff --git a/c_src/workitems.cc b/c_src/workitems.cc
@@ -230,7 +230,7 @@ MoveTask::DoWork()
         struct timeval tv;
 
         gettimeofday(&tv, NULL);
-        
+
         // 14400 is 4 hours in seconds ... 60*60*4
         if ((m_ItrWrap->m_LastLogReport + 14400) < tv.tv_sec && NULL!=m_ItrWrap->get())
         {
@@ -272,6 +272,9 @@ MoveTask::DoWork()
 
     }   // switch
 
+    // set state for Erlang side to read
+    m_ItrWrap->SetValid(itr->Valid());
+
     // Post processing before telling the world the results
     //  (while only one thread might be looking at objects)
     if (m_ItrWrap->m_Options.iterator_refresh)