From f81b6fbcd61b5aac6bb2dcce346f50b9549d52ba Mon Sep 17 00:00:00 2001 From: Peter Johnson Date: Mon, 2 Oct 2017 13:06:39 -0700 Subject: [PATCH] Fix handling of deleted values in several places. (#241) Now that the m_entries entry is kept, other places can't assume that a value exists simply because the entry exists. --- src/main/native/cpp/Storage.cpp | 42 +++++++++++++------------- src/main/native/cpp/Storage.h | 2 +- src/test/native/cpp/StorageTest.cpp | 46 ++++++++++++++++++++++++++--- 3 files changed, 63 insertions(+), 27 deletions(-) diff --git a/src/main/native/cpp/Storage.cpp b/src/main/native/cpp/Storage.cpp index 7f2083c..bb51481 100644 --- a/src/main/native/cpp/Storage.cpp +++ b/src/main/native/cpp/Storage.cpp @@ -123,11 +123,10 @@ void Storage::ProcessIncomingEntryAssign(std::shared_ptr msg, entry = m_idmap[id]; if (!entry) { // create local - bool is_new; - entry = GetOrNew(name, &is_new); + entry = GetOrNew(name); entry->id = id; m_idmap[id] = entry; - if (is_new) { + if (!entry->value) { // didn't exist at all (rather than just being a response to a // id assignment request) entry->value = msg->value(); @@ -377,6 +376,7 @@ void Storage::GetInitialAssignments( conn.set_state(INetworkConnection::kSynchronized); for (auto& i : m_entries) { Entry* entry = i.getValue(); + if (!entry->value) continue; msgs->emplace_back(Message::EntryAssign(i.getKey(), entry->id, entry->seq_num.value(), entry->value, entry->flags)); @@ -415,9 +415,8 @@ void Storage::ApplyInitialAssignments( SequenceNumber seq_num(msg->seq_num_uid()); StringRef name = msg->str(); - bool is_new; - Entry* entry = GetOrNew(name, &is_new); - if (is_new) { + Entry* entry = GetOrNew(name); + if (!entry->value) { // doesn't currently exist entry->value = msg->value(); entry->flags = msg->flags(); @@ -628,7 +627,7 @@ void Storage::SetEntryFlags(unsigned int id_local, unsigned int flags) { void Storage::SetEntryFlagsImpl(Entry* entry, unsigned int flags, std::unique_lock& lock, bool local) { - if (entry->flags == flags) return; + if (!entry->value || entry->flags == flags) return; // update persistent dirty flag if persistent flag changed if ((entry->flags & NT_PERSISTENT) != (flags & NT_PERSISTENT)) @@ -700,6 +699,9 @@ void Storage::DeleteEntryImpl(Entry* entry, std::unique_lock& lock, // update persistent dirty flag if it's a persistent value if (entry->IsPersistent()) m_persistent_dirty = true; + // reset flags + entry->flags = 0; + if (!old_value) return; // was not previously assigned // notify @@ -720,7 +722,7 @@ template void Storage::DeleteAllEntriesImpl(bool local, F should_delete) { for (auto& i : m_entries) { Entry* entry = i.getValue(); - if (should_delete(entry)) { + if (entry->value && should_delete(entry)) { // notify it's being deleted m_notifier.NotifyEntry(entry->local_id, i.getKey(), entry->value, NT_NOTIFY_DELETE | (local ? NT_NOTIFY_LOCAL : 0)); @@ -753,15 +755,12 @@ void Storage::DeleteAllEntries() { dispatcher->QueueOutgoing(Message::ClearEntries(), nullptr, nullptr); } -Storage::Entry* Storage::GetOrNew(StringRef name, bool* is_new) { +Storage::Entry* Storage::GetOrNew(StringRef name) { auto& entry = m_entries[name]; if (!entry) { - if (is_new) *is_new = true; m_localmap.emplace_back(new Entry(name)); entry = m_localmap.back().get(); entry->local_id = m_localmap.size() - 1; - } else { - if (is_new) *is_new = false; } return entry; } @@ -777,10 +776,10 @@ std::vector Storage::GetEntries(StringRef prefix, std::lock_guard lock(m_mutex); std::vector ids; for (auto& i : m_entries) { - if (!i.getKey().startswith(prefix)) continue; Entry* entry = i.getValue(); - if (types != 0 && (!entry->value || (types & entry->value->type()) == 0)) - continue; + auto value = entry->value.get(); + if (!value || !i.getKey().startswith(prefix)) continue; + if (types != 0 && (types & value->type()) == 0) continue; ids.push_back(entry->local_id); } return ids; @@ -833,10 +832,9 @@ std::vector Storage::GetEntryInfo(int inst, StringRef prefix, std::lock_guard lock(m_mutex); std::vector infos; for (auto& i : m_entries) { - if (!i.getKey().startswith(prefix)) continue; Entry* entry = i.getValue(); - auto value = entry->value; - if (!value) continue; + auto value = entry->value.get(); + if (!value || !i.getKey().startswith(prefix)) continue; if (types != 0 && (types & value->type()) == 0) continue; EntryInfo info; info.entry = Handle(inst, entry->local_id, Handle::kEntry); @@ -858,8 +856,8 @@ unsigned int Storage::AddListener( // perform immediate notifications if ((flags & NT_NOTIFY_IMMEDIATE) != 0 && (flags & NT_NOTIFY_NEW) != 0) { for (auto& i : m_entries) { - if (!i.getKey().startswith(prefix)) continue; Entry* entry = i.getValue(); + if (!entry->value || !i.getKey().startswith(prefix)) continue; m_notifier.NotifyEntry(entry->local_id, i.getKey(), entry->value, NT_NOTIFY_IMMEDIATE | NT_NOTIFY_NEW, uid); } @@ -877,7 +875,6 @@ unsigned int Storage::AddListener( if ((flags & NT_NOTIFY_IMMEDIATE) != 0 && (flags & NT_NOTIFY_NEW) != 0 && local_id < m_localmap.size()) { Entry* entry = m_localmap[local_id].get(); - // if no value, don't notify if (entry->value) { m_notifier.NotifyEntry(local_id, entry->name, entry->value, NT_NOTIFY_IMMEDIATE | NT_NOTIFY_NEW, uid); @@ -895,6 +892,7 @@ unsigned int Storage::AddPolledListener(unsigned int poller, StringRef prefix, for (auto& i : m_entries) { if (!i.getKey().startswith(prefix)) continue; Entry* entry = i.getValue(); + if (!entry->value) continue; m_notifier.NotifyEntry(entry->local_id, i.getKey(), entry->value, NT_NOTIFY_IMMEDIATE | NT_NOTIFY_NEW, uid); } @@ -934,7 +932,7 @@ bool Storage::GetPersistentEntries( for (auto& i : m_entries) { Entry* entry = i.getValue(); // only write persistent-flagged values - if (!entry->IsPersistent()) continue; + if (!entry->value || !entry->IsPersistent()) continue; entries->emplace_back(i.getKey(), entry->value); } } @@ -959,7 +957,7 @@ bool Storage::GetEntries( for (auto& i : m_entries) { Entry* entry = i.getValue(); // only write values with given prefix - if (!i.getKey().startswith(prefix)) continue; + if (!entry->value || !i.getKey().startswith(prefix)) continue; entries->emplace_back(i.getKey(), entry->value); } } diff --git a/src/main/native/cpp/Storage.h b/src/main/native/cpp/Storage.h index 44aae5a..9bf4201 100644 --- a/src/main/native/cpp/Storage.h +++ b/src/main/native/cpp/Storage.h @@ -251,7 +251,7 @@ class Storage : public IStorage { template void DeleteAllEntriesImpl(bool local, F should_delete); void DeleteAllEntriesImpl(bool local); - Entry* GetOrNew(StringRef name, bool* is_new = nullptr); + Entry* GetOrNew(StringRef name); }; } // namespace nt diff --git a/src/test/native/cpp/StorageTest.cpp b/src/test/native/cpp/StorageTest.cpp index 2241ed8..603489b 100644 --- a/src/test/native/cpp/StorageTest.cpp +++ b/src/test/native/cpp/StorageTest.cpp @@ -948,17 +948,55 @@ TEST_P(StorageTestPopulateOne, ProcessIncomingEntryAssignWithFlags) { 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)); + ::testing::Mock::VerifyAndClearExpectations(&dispatcher); + ::testing::Mock::VerifyAndClearExpectations(¬ifier); + auto handle2 = storage.GetEntry("foo"); ASSERT_EQ(handle, handle2); } +TEST_P(StorageTestPopulateOne, DeletedEntryFlags) { + EXPECT_CALL(dispatcher, QueueOutgoing(_, _, _)).Times(AnyNumber()); + EXPECT_CALL(notifier, NotifyEntry(_, _, _, _, _)).Times(AnyNumber()); + auto handle = storage.GetEntry("foo"); + storage.SetEntryFlags("foo", 2); + storage.DeleteEntry("foo"); + ::testing::Mock::VerifyAndClearExpectations(&dispatcher); + ::testing::Mock::VerifyAndClearExpectations(¬ifier); + + EXPECT_EQ(storage.GetEntryFlags("foo"), 0u); + EXPECT_EQ(storage.GetEntryFlags(handle), 0u); + storage.SetEntryFlags("foo", 4); + storage.SetEntryFlags(handle, 4); + EXPECT_EQ(storage.GetEntryFlags("foo"), 0u); + EXPECT_EQ(storage.GetEntryFlags(handle), 0u); +} + +TEST_P(StorageTestPopulateOne, DeletedDeleteAllEntries) { + EXPECT_CALL(dispatcher, QueueOutgoing(_, _, _)).Times(AnyNumber()); + EXPECT_CALL(notifier, NotifyEntry(_, _, _, _, _)).Times(AnyNumber()); + storage.DeleteEntry("foo"); + ::testing::Mock::VerifyAndClearExpectations(&dispatcher); + ::testing::Mock::VerifyAndClearExpectations(¬ifier); + + EXPECT_CALL(dispatcher, QueueOutgoing(MessageEq(Message::ClearEntries()), + IsNull(), IsNull())); + storage.DeleteAllEntries(); +} + +TEST_P(StorageTestPopulateOne, DeletedGetEntries) { + EXPECT_CALL(dispatcher, QueueOutgoing(_, _, _)).Times(AnyNumber()); + EXPECT_CALL(notifier, NotifyEntry(_, _, _, _, _)).Times(AnyNumber()); + storage.DeleteEntry("foo"); + ::testing::Mock::VerifyAndClearExpectations(&dispatcher); + ::testing::Mock::VerifyAndClearExpectations(¬ifier); + + EXPECT_TRUE(storage.GetEntries("", 0).empty()); +} + INSTANTIATE_TEST_CASE_P(StorageTestsEmpty, StorageTestEmpty, ::testing::Bool(), ); INSTANTIATE_TEST_CASE_P(StorageTestsPopulateOne, StorageTestPopulateOne,