Skip to content

Commit eba79a5

Browse files
jamesmulcahyjamesmulcahy
committed
Address review comments
Signed-off-by: James Mulcahy <[email protected]>
1 parent 083f606 commit eba79a5

File tree

6 files changed

+65
-25
lines changed

6 files changed

+65
-25
lines changed

‎include/proxy-wasm/context.h

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -353,9 +353,10 @@ class ContextBase : public RootInterface,
353353
WasmResult getSharedData(std::string_view key,
354354
std::pair<std::string, uint32_t /* cas */> *data) override;
355355
WasmResult setSharedData(std::string_view key, std::string_view value, uint32_t cas) override;
356-
WasmResult getSharedDataKeys(std::string_view key_prefix, std::vector<std::string> *result) override;
357-
WasmResult removeSharedDataKey(std::string_view key, uint32_t cas) override;
358-
356+
WasmResult getSharedDataKeys(std::string_view key_prefix,
357+
std::vector<std::string> *result) override;
358+
WasmResult removeSharedDataKey(std::string_view key, uint32_t cas,
359+
std::pair<std::string, uint32_t> *result) override;
359360

360361
// Shared Queue
361362
WasmResult registerSharedQueue(std::string_view queue_name,

‎include/proxy-wasm/context_interface.h

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -627,15 +627,19 @@ struct SharedDataInterface {
627627
* @param key_prefix is used to restrict the results to keys matching the given prefix
628628
* @param data is a location to store the returned value.
629629
*/
630-
virtual WasmResult getSharedDataKeys(std::string_view key_prefix, std::vector<std::string> *result) = 0;
630+
virtual WasmResult getSharedDataKeys(std::string_view key_prefix,
631+
std::vector<std::string> *result) = 0;
631632

632633
/**
633634
* Removes the given key from the data shared between VMs.
634635
* @param key is a proxy-wide key mapping to the shared data value.
635636
* @param cas is a compare-and-swap value. If it is zero it is ignored, otherwise it must match
637+
* @param cas is a location to store value, and cas number, associated with the removed key
636638
* the cas associated with the value.
637639
*/
638-
virtual WasmResult removeSharedDataKey(std::string_view key, uint32_t cas) = 0;
640+
virtual WasmResult
641+
removeSharedDataKey(std::string_view key, uint32_t cas,
642+
std::pair<std::string /* value */, uint32_t /* cas */> *result) = 0;
639643
}; // namespace proxy_wasm
640644

641645
struct SharedQueueInterface {

‎src/context.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -197,11 +197,11 @@ WasmResult ContextBase::getSharedDataKeys(std::string_view key_prefix,
197197
return getGlobalSharedData().keys(wasm_->vm_id(), key_prefix, result);
198198
}
199199

200-
WasmResult ContextBase::removeSharedDataKey(std::string_view key, uint32_t cas) {
201-
return getGlobalSharedData().remove(wasm_->vm_id(), key, cas);
200+
WasmResult ContextBase::removeSharedDataKey(std::string_view key, uint32_t cas,
201+
std::pair<std::string, uint32_t> *result) {
202+
return getGlobalSharedData().remove(wasm_->vm_id(), key, cas, result);
202203
}
203204

204-
205205
// Shared Queue
206206

207207
WasmResult ContextBase::registerSharedQueue(std::string_view queue_name,

‎src/shared_data.cc

Lines changed: 20 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,12 @@ WasmResult SharedData::get(std::string_view vm_id, const std::string_view key,
5858

5959
WasmResult SharedData::keys(std::string_view vm_id, const std::string_view key_prefix,
6060
std::vector<std::string> *result) {
61+
result->clear();
62+
6163
std::lock_guard<std::mutex> lock(mutex_);
6264
auto map = data_.find(std::string(vm_id));
6365
if (map == data_.end()) {
64-
return WasmResult::NotFound;
66+
return WasmResult::Ok;
6567
}
6668

6769
for (auto kv : map->second) {
@@ -73,46 +75,52 @@ WasmResult SharedData::keys(std::string_view vm_id, const std::string_view key_p
7375
return WasmResult::Ok;
7476
}
7577

76-
WasmResult SharedData::remove(std::string_view vm_id, std::string_view key, uint32_t cas) {
78+
WasmResult SharedData::set(std::string_view vm_id, std::string_view key, std::string_view value,
79+
uint32_t cas) {
7780
std::lock_guard<std::mutex> lock(mutex_);
7881
std::unordered_map<std::string, std::pair<std::string, uint32_t>> *map;
7982
auto map_it = data_.find(std::string(vm_id));
8083
if (map_it == data_.end()) {
81-
return WasmResult::Ok;
84+
map = &data_[std::string(vm_id)];
8285
} else {
8386
map = &map_it->second;
8487
}
85-
8688
auto it = map->find(std::string(key));
8789
if (it != map->end()) {
8890
if (cas && cas != it->second.second) {
8991
return WasmResult::CasMismatch;
9092
}
91-
map->erase(it);
93+
it->second = std::make_pair(std::string(value), nextCas());
94+
} else {
95+
map->emplace(key, std::make_pair(std::string(value), nextCas()));
9296
}
9397
return WasmResult::Ok;
9498
}
9599

96-
WasmResult SharedData::set(std::string_view vm_id, std::string_view key, std::string_view value,
97-
uint32_t cas) {
100+
WasmResult SharedData::remove(std::string_view vm_id, std::string_view key, uint32_t cas,
101+
std::pair<std::string, uint32_t> *result) {
98102
std::lock_guard<std::mutex> lock(mutex_);
99103
std::unordered_map<std::string, std::pair<std::string, uint32_t>> *map;
100104
auto map_it = data_.find(std::string(vm_id));
101105
if (map_it == data_.end()) {
102-
map = &data_[std::string(vm_id)];
106+
return WasmResult::NotFound;
103107
} else {
104108
map = &map_it->second;
105109
}
110+
106111
auto it = map->find(std::string(key));
107112
if (it != map->end()) {
108113
if (cas && cas != it->second.second) {
109114
return WasmResult::CasMismatch;
110115
}
111-
it->second = std::make_pair(std::string(value), nextCas());
112-
} else {
113-
map->emplace(key, std::make_pair(std::string(value), nextCas()));
116+
if (result != nullptr) {
117+
// printf("Here w/ %s", it->second.first.c_str());
118+
*result = it->second;
119+
}
120+
map->erase(it);
121+
return WasmResult::Ok;
114122
}
115-
return WasmResult::Ok;
123+
return WasmResult::NotFound;
116124
}
117125

118126
} // namespace proxy_wasm

‎src/shared_data.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,10 @@ class SharedData {
2727
std::pair<std::string, uint32_t> *result);
2828
WasmResult keys(std::string_view vm_id, const std::string_view key_prefix,
2929
std::vector<std::string> *result);
30-
WasmResult remove(std::string_view vm_id, const std::string_view key, uint32_t cas);
3130
WasmResult set(std::string_view vm_id, std::string_view key, std::string_view value,
3231
uint32_t cas);
32+
WasmResult remove(std::string_view vm_id, const std::string_view key, uint32_t cas,
33+
std::pair<std::string, uint32_t> *result);
3334
void deleteByVmId(std::string_view vm_id);
3435

3536
private:

‎test/shared_data_test.cc

Lines changed: 30 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,24 @@ namespace proxy_wasm {
2424

2525
TEST(SharedData, SingleThread) {
2626
SharedData shared_data(false);
27+
std::string_view vm_id = "id";
28+
29+
// Validate we get an 'Ok' response when fetching keys before anything
30+
// is initialized.
31+
std::vector<std::string> keys;
32+
EXPECT_EQ(WasmResult::Ok, shared_data.keys(vm_id, "", &keys));
33+
EXPECT_EQ(0, keys.size());
34+
35+
// Validate that we clear the result set
36+
std::vector<std::string> nonEmptyKeys(2);
37+
nonEmptyKeys[0] = "valueA";
38+
nonEmptyKeys[1] = "valueB";
39+
EXPECT_EQ(WasmResult::Ok, shared_data.keys(vm_id, "", &nonEmptyKeys));
40+
EXPECT_EQ(0, nonEmptyKeys.size());
41+
2742
std::pair<std::string, uint32_t> result;
2843
EXPECT_EQ(WasmResult::NotFound, shared_data.get("non-exist", "non-exists", &result));
2944

30-
std::string_view vm_id = "id";
3145
std::string_view key = "key";
3246
std::string_view value = "1";
3347
EXPECT_EQ(WasmResult::Ok, shared_data.set(vm_id, key, value, 0));
@@ -45,7 +59,6 @@ TEST(SharedData, SingleThread) {
4559
EXPECT_EQ(value, result.first);
4660
EXPECT_EQ(result.second, 3);
4761

48-
std::vector<std::string> keys;
4962
EXPECT_EQ(WasmResult::Ok, shared_data.keys(vm_id, "unmatched-prefix", &keys));
5063
EXPECT_EQ(0, keys.size());
5164

@@ -57,12 +70,25 @@ TEST(SharedData, SingleThread) {
5770
EXPECT_EQ(key, keys[0]);
5871

5972
keys.clear();
60-
EXPECT_EQ(WasmResult::CasMismatch, shared_data.remove(vm_id, key, 911));
73+
EXPECT_EQ(WasmResult::CasMismatch, shared_data.remove(vm_id, key, 911, nullptr));
6174
EXPECT_EQ(WasmResult::Ok, shared_data.keys(vm_id, "ke", &keys));
6275
EXPECT_EQ(1, keys.size());
6376

64-
EXPECT_EQ(WasmResult::Ok, shared_data.remove(vm_id, key, 0));
77+
EXPECT_EQ(WasmResult::Ok, shared_data.remove(vm_id, key, 0, nullptr));
6578
EXPECT_EQ(WasmResult::NotFound, shared_data.get(vm_id, key, &result));
79+
80+
EXPECT_EQ(WasmResult::NotFound, shared_data.remove(vm_id, "non-existent_key", 0, nullptr));
81+
82+
EXPECT_EQ(WasmResult::Ok, shared_data.set(vm_id, key, value, 0));
83+
EXPECT_EQ(WasmResult::Ok, shared_data.set(vm_id, key, value, 0));
84+
EXPECT_EQ(WasmResult::Ok, shared_data.get(vm_id, key, &result));
85+
86+
uint32_t expectedCasValue = result.second;
87+
88+
std::pair<std::string, uint32_t> removeResult;
89+
EXPECT_EQ(WasmResult::Ok, shared_data.remove(vm_id, key, 0, &removeResult));
90+
EXPECT_EQ(value, removeResult.first);
91+
EXPECT_EQ(removeResult.second, expectedCasValue);
6692
}
6793

6894
void incrementData(SharedData *shared_data, std::string_view vm_id, std::string_view key) {

0 commit comments

Comments
 (0)