Don't actually remove deleted entries from m_entries. (#239)

Doing so invalidates entry instance handles.

Fixes #238.

src/main/native/cpp/Storage.cpp

@@ -282,7 +282,7 @@ void Storage::ProcessIncomingEntryDelete(std::shared_ptr<Message> msg,
   }
 
   // update local
-  DeleteEntryImpl(m_idmap[id], m_entries.end(), lock, false);
+  DeleteEntryImpl(m_idmap[id], lock, false);
 
   // broadcast to all other connections (note for client there won't
   // be any other connections, so don't bother)
@@ -669,23 +669,20 @@ void Storage::DeleteEntry(StringRef name) {
   std::unique_lock<std::mutex> lock(m_mutex);
   auto i = m_entries.find(name);
   if (i == m_entries.end()) return;
-  DeleteEntryImpl(i->getValue(), i, lock, true);
+  DeleteEntryImpl(i->getValue(), lock, true);
 }
 
 void Storage::DeleteEntry(unsigned int local_id) {
   std::unique_lock<std::mutex> lock(m_mutex);
   if (local_id >= m_localmap.size()) return;
-  DeleteEntryImpl(m_localmap[local_id].get(), m_entries.end(), lock, true);
+  DeleteEntryImpl(m_localmap[local_id].get(), lock, true);
 }
 
-void Storage::DeleteEntryImpl(Entry* entry, EntriesMap::iterator it,
-                              std::unique_lock<std::mutex>& lock, bool local) {
+void Storage::DeleteEntryImpl(Entry* entry, std::unique_lock<std::mutex>& lock,
+                              bool local) {
   unsigned int id = entry->id;
 
-  // Erase entry from name and id mappings.
-  // Get iterator if it wasn't provided.
-  if (it == m_entries.end()) it = m_entries.find(entry->name);
-  if (it != m_entries.end()) m_entries.erase(it);
+  // Erase entry from id mapping.
   if (id < m_idmap.size()) m_idmap[id] = nullptr;
 
   // empty the value and reset id and local_write flag
@@ -721,10 +718,6 @@ void Storage::DeleteEntryImpl(Entry* entry, EntriesMap::iterator it,
 
 template <typename F>
 void Storage::DeleteAllEntriesImpl(bool local, F should_delete) {
-  if (m_entries.empty()) return;
-
-  // can't erase without invalidating iterators, so build a new map
-  EntriesMap entries;
   for (auto& i : m_entries) {
     Entry* entry = i.getValue();
     if (should_delete(entry)) {
@@ -738,11 +731,7 @@ void Storage::DeleteAllEntriesImpl(bool local, F should_delete) {
       entry->value.reset();
       continue;
     }
-
-    // add it to new entries
-    entries.insert(std::make_pair(i.getKey(), std::move(i.getValue())));
   }
-  m_entries.swap(entries);
 }
 
 void Storage::DeleteAllEntriesImpl(bool local) {

src/main/native/cpp/Storage.h

@@ -221,8 +221,8 @@ class Storage : public IStorage {
                          std::unique_lock<std::mutex>& lock, bool local);
   void SetEntryFlagsImpl(Entry* entry, unsigned int flags,
                          std::unique_lock<std::mutex>& lock, bool local);
-  void DeleteEntryImpl(Entry* entry, EntriesMap::iterator it,
-                       std::unique_lock<std::mutex>& lock, bool local);
+  void DeleteEntryImpl(Entry* entry, std::unique_lock<std::mutex>& lock,
+                       bool local);
 
   // Must be called with m_mutex held
   template <typename F>

src/test/native/cpp/StorageTest.cpp

@@ -462,7 +462,10 @@ TEST_P(StorageTestPopulated, DeleteEntryExist) {
                           NT_NOTIFY_DELETE | NT_NOTIFY_LOCAL, UINT_MAX));
 
   storage.DeleteEntry("foo2");
-  EXPECT_TRUE(entries().count("foo2") == 0);
+  ASSERT_EQ(1u, entries().count("foo2"));
+  EXPECT_EQ(nullptr, entries()["foo2"]->value);
+  EXPECT_EQ(0xffffu, entries()["foo2"]->id);
+  EXPECT_FALSE(entries()["foo2"]->local_write);
   if (GetParam()) {
     ASSERT_TRUE(idmap().size() >= 2);
     EXPECT_FALSE(idmap()[1]);
@@ -482,7 +485,8 @@ TEST_P(StorageTestPopulated, DeleteAllEntries) {
       .Times(4);
 
   storage.DeleteAllEntries();
-  ASSERT_TRUE(entries().empty());
+  ASSERT_EQ(1u, entries().count("foo2"));
+  EXPECT_EQ(nullptr, entries()["foo2"]->value);
 }
 
 TEST_P(StorageTestPopulated, DeleteAllEntriesPersistent) {
@@ -495,8 +499,8 @@ TEST_P(StorageTestPopulated, DeleteAllEntriesPersistent) {
       .Times(3);
 
   storage.DeleteAllEntries();
-  ASSERT_EQ(1u, entries().size());
-  EXPECT_EQ(1u, entries().count("foo2"));
+  ASSERT_EQ(1u, entries().count("foo2"));
+  EXPECT_NE(nullptr, entries()["foo2"]->value);
 }
 
 TEST_P(StorageTestPopulated, GetEntryInfoAll) {
@@ -941,6 +945,20 @@ TEST_P(StorageTestPopulateOne, ProcessIncomingEntryAssignWithFlags) {
                           conn.get(), conn);
 }
 
+TEST_P(StorageTestPopulateOne, DeleteCheckHandle) {
+  EXPECT_CALL(dispatcher, QueueOutgoing(_, _, _)).Times(AnyNumber());
+  EXPECT_CALL(notifier, NotifyEntry(_, _, _, _, _)).Times(AnyNumber());
+  EXPECT_CALL(notifier, local_notifiers())
+      .Times(AnyNumber())
+      .WillRepeatedly(Return(false));
+
+  auto handle = storage.GetEntry("foo");
+  storage.DeleteEntry("foo");
+  storage.SetEntryTypeValue("foo", Value::MakeBoolean(true));
+  auto handle2 = storage.GetEntry("foo");
+  ASSERT_EQ(handle, handle2);
+}
+
 INSTANTIATE_TEST_CASE_P(StorageTestsEmpty, StorageTestEmpty,
                         ::testing::Bool(), );
 INSTANTIATE_TEST_CASE_P(StorageTestsPopulateOne, StorageTestPopulateOne,