Skip to content
This repository has been archived by the owner on Sep 21, 2020. It is now read-only.

Commit

Permalink
Fix handling of deleted values in several places. (#241)
Browse files Browse the repository at this point in the history 
Now that the m_entries entry is kept, other places can't assume that
a value exists simply because the entry exists.
  • Loading branch information
@PeterJohnson
PeterJohnson authored Oct 2, 2017
1 parent 303df62 commit f81b6fb
Show file tree
Hide file tree
Showing 3 changed files with 63 additions and 27 deletions.
42 changes: 20 additions & 22 deletions src/main/native/cpp/Storage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,10 @@ void Storage::ProcessIncomingEntryAssign(std::shared_ptr<Message> 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();
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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<std::mutex>& 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))
Expand Down Expand Up @@ -700,6 +699,9 @@ void Storage::DeleteEntryImpl(Entry* entry, std::unique_lock<std::mutex>& 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
Expand All @@ -720,7 +722,7 @@ template <typename F>
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));
Expand Down Expand Up @@ -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;
}
Expand All @@ -777,10 +776,10 @@ std::vector<unsigned int> Storage::GetEntries(StringRef prefix,
std::lock_guard<std::mutex> lock(m_mutex);
std::vector<unsigned int> 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;
Expand Down Expand Up @@ -833,10 +832,9 @@ std::vector<EntryInfo> Storage::GetEntryInfo(int inst, StringRef prefix,
std::lock_guard<std::mutex> lock(m_mutex);
std::vector<EntryInfo> 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);
Expand All @@ -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);
}
Expand All @@ -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);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
}
}
Expand All @@ -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);
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/native/cpp/Storage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,7 @@ class Storage : public IStorage {
template <typename F>
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
Expand Down
46 changes: 42 additions & 4 deletions src/test/native/cpp/StorageTest.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(&notifier);

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(&notifier);

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(&notifier);

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(&notifier);

EXPECT_TRUE(storage.GetEntries("", 0).empty());
}

INSTANTIATE_TEST_CASE_P(StorageTestsEmpty, StorageTestEmpty,
::testing::Bool(), );
INSTANTIATE_TEST_CASE_P(StorageTestsPopulateOne, StorageTestPopulateOne,
Expand Down

0 comments on commit f81b6fb

Please sign in to comment.