Skip to content

Commit b38b44f

Browse files
johnlannijohnlanni
committed
reuse plugin handle when recover
Link: https://code.alibaba-inc.com/Ingress/proxy-wasm-cpp-host/codereview/14314408 * reuse plugin handle when recover
1 parent 23709f2 commit b38b44f

File tree

3 files changed

+67
-83
lines changed

3 files changed

+67
-83
lines changed

‎include/proxy-wasm/wasm.h

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -395,22 +395,20 @@ class PluginHandleBase : public std::enable_shared_from_this<PluginHandleBase> {
395395
std::shared_ptr<WasmBase> &wasm() { return wasm_handle_->wasm(); }
396396
std::shared_ptr<WasmHandleBase> &wasmHandle() { return wasm_handle_; }
397397

398-
void setRecoverPluginCallback(
399-
std::function<std::shared_ptr<PluginHandleBase>(std::shared_ptr<WasmHandleBase> &)> &&f) {
398+
void setRecoverPluginCallback(std::function<bool(std::shared_ptr<WasmHandleBase> &)> &&f) {
400399
recover_plugin_callback_ = std::move(f);
401400
}
402401
void setNeedRecover() { need_recover_ = true; }
403402
bool needRecover() { return need_recover_; }
404-
bool doRecover(std::shared_ptr<PluginHandleBase> &new_handle) {
403+
bool doRecover() {
405404
if (!need_recover_ || recover_plugin_callback_ == nullptr) {
406405
return true;
407406
}
408407
std::shared_ptr<WasmHandleBase> new_wasm_handle;
409408
if (!wasm_handle_->doRecover(new_wasm_handle)) {
410409
return false;
411410
}
412-
new_handle = recover_plugin_callback_(new_wasm_handle);
413-
if (!new_handle) {
411+
if (!recover_plugin_callback_(new_wasm_handle)) {
414412
return false;
415413
}
416414
need_recover_ = false;
@@ -425,8 +423,7 @@ class PluginHandleBase : public std::enable_shared_from_this<PluginHandleBase> {
425423
bool need_recover_ = false;
426424
std::shared_ptr<PluginBase> plugin_;
427425
std::shared_ptr<WasmHandleBase> wasm_handle_;
428-
std::function<std::shared_ptr<PluginHandleBase>(std::shared_ptr<WasmHandleBase> &)>
429-
recover_plugin_callback_;
426+
std::function<bool(std::shared_ptr<WasmHandleBase> &)> recover_plugin_callback_;
430427
};
431428

432429
using PluginHandleFactory = std::function<std::shared_ptr<PluginHandleBase>(

‎src/wasm.cc

Lines changed: 29 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -675,43 +675,35 @@ void setPluginRecoverCallback(const std::string &key,
675675
const std::shared_ptr<WasmHandleBase> &base_handle,
676676
const std::shared_ptr<PluginBase> &plugin) {
677677
std::weak_ptr<PluginHandleBase> plugin_handle_for_copy = plugin_handle;
678-
plugin_handle->setRecoverPluginCallback(
679-
[key, plugin_handle_for_copy, base_handle,
680-
plugin](std::shared_ptr<WasmHandleBase> &wasm_handle) -> std::shared_ptr<PluginHandleBase> {
681-
const auto &integration = base_handle->wasm()->wasm_vm()->integration();
682-
integration->trace("Start recover plugin_handle");
683-
auto it = local_plugins.find(key);
684-
if (it != local_plugins.end()) {
685-
auto plugin_handle = it->second.lock();
686-
if (plugin_handle) {
687-
integration->trace("Plugin handle already exists");
688-
return plugin_handle;
689-
}
690-
local_plugins.erase(key);
691-
}
692-
auto plugin_handle = plugin_handle_for_copy.lock();
693-
if (!plugin_handle) {
694-
base_handle->wasm()->fail(FailState::RecoverError, "Plugin handle lock failed");
695-
return nullptr;
696-
}
697-
plugin_handle->updateWasm(wasm_handle);
698-
// Create and initialize new thread-local Plugin.
699-
auto *plugin_context = wasm_handle->wasm()->start(plugin);
700-
if (plugin_context == nullptr) {
701-
base_handle->wasm()->fail(FailState::RecoverError,
702-
"Failed to start thread-local Wasm during recover");
703-
return nullptr;
704-
}
705-
if (!wasm_handle->wasm()->configure(plugin_context, plugin)) {
706-
base_handle->wasm()->fail(FailState::RecoverError,
707-
"Failed to configure thread-local Wasm plugin during recover");
708-
return nullptr;
709-
}
710-
local_plugins[key] = plugin_handle;
711-
integration->trace("Plugin_handle has been recovered");
712-
setPluginFailCallback(key, wasm_handle, plugin_handle);
713-
return plugin_handle;
714-
});
678+
plugin_handle->setRecoverPluginCallback([key, plugin_handle_for_copy, base_handle,
679+
plugin](std::shared_ptr<WasmHandleBase> &wasm_handle) {
680+
const auto &integration = base_handle->wasm()->wasm_vm()->integration();
681+
integration->trace("Start recover plugin_handle");
682+
// We cannot reuse plugin that other have created because the life cycle of the plugin
683+
// handle is controlled by the upper layer.
684+
auto plugin_handle = plugin_handle_for_copy.lock();
685+
if (!plugin_handle) {
686+
base_handle->wasm()->fail(FailState::RecoverError, "Plugin handle lock failed");
687+
return false;
688+
}
689+
plugin_handle->updateWasm(wasm_handle);
690+
// Create and initialize new thread-local Plugin.
691+
auto *plugin_context = wasm_handle->wasm()->start(plugin);
692+
if (plugin_context == nullptr) {
693+
base_handle->wasm()->fail(FailState::RecoverError,
694+
"Failed to start thread-local Wasm during recover");
695+
return false;
696+
}
697+
if (!wasm_handle->wasm()->configure(plugin_context, plugin)) {
698+
base_handle->wasm()->fail(FailState::RecoverError,
699+
"Failed to configure thread-local Wasm plugin during recover");
700+
return false;
701+
}
702+
local_plugins[key] = plugin_handle;
703+
integration->trace("Plugin_handle has been recovered");
704+
setPluginFailCallback(key, wasm_handle, plugin_handle);
705+
return true;
706+
});
715707
}
716708

717709
std::shared_ptr<PluginHandleBase> getOrCreateThreadLocalPlugin(

‎test/wasm_test.cc

Lines changed: 34 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -162,33 +162,28 @@ TEST_P(TestVm, RecoverCrashedThreadLocalWasm) {
162162
// Create a thread local plugin.
163163
auto thread_local_plugin = getOrCreateThreadLocalPlugin(
164164
base_wasm_handle, plugin, wasm_handle_clone_factory, plugin_handle_factory);
165+
auto old_wasm_ptr = thread_local_plugin->wasm();
165166
// Cause runtime crash.
166167
thread_local_plugin->wasm()->wasm_vm()->fail(FailState::RuntimeError, "runtime error msg");
167168
ASSERT_TRUE(thread_local_plugin->wasm()->isFailed());
168169

169-
auto old_wasm_ptr = thread_local_plugin->wasm();
170170
// do recover.
171-
std::shared_ptr<PluginHandleBase> new_thread_local_plugin;
172-
ASSERT_TRUE(thread_local_plugin->doRecover(new_thread_local_plugin));
171+
ASSERT_TRUE(thread_local_plugin->doRecover());
173172
// Verify recover success.
174-
ASSERT_FALSE(new_thread_local_plugin->wasm()->isFailed());
175-
// Verify handle is reused.
176-
ASSERT_EQ(new_thread_local_plugin->wasmHandle().get(), thread_local_plugin->wasmHandle().get());
177-
ASSERT_EQ(new_thread_local_plugin.get(), thread_local_plugin.get());
173+
ASSERT_FALSE(thread_local_plugin->wasm()->isFailed());
178174
// Verify the pointer to WasmBase is different from the crashed one.
179-
ASSERT_NE(new_thread_local_plugin->wasm(), old_wasm_ptr);
175+
ASSERT_NE(thread_local_plugin->wasm(), old_wasm_ptr);
180176

177+
old_wasm_ptr = thread_local_plugin->wasm();
181178
// Cause runtime crash again.
182-
new_thread_local_plugin->wasm()->wasm_vm()->fail(FailState::RuntimeError, "runtime error msg");
183-
ASSERT_TRUE(new_thread_local_plugin->wasm()->isFailed());
184-
old_wasm_ptr = new_thread_local_plugin->wasm();
179+
thread_local_plugin->wasm()->wasm_vm()->fail(FailState::RuntimeError, "runtime error msg");
180+
ASSERT_TRUE(thread_local_plugin->wasm()->isFailed());
185181
// Do recover again.
186-
std::shared_ptr<PluginHandleBase> new_thread_local_plugin2;
187-
ASSERT_TRUE(new_thread_local_plugin->doRecover(new_thread_local_plugin2));
182+
ASSERT_TRUE(thread_local_plugin->doRecover());
188183
// Verify recover again success.
189-
ASSERT_FALSE(new_thread_local_plugin2->wasm()->isFailed());
184+
ASSERT_FALSE(thread_local_plugin->wasm()->isFailed());
190185
// Verify the pointer to WasmBase is different from the crashed one.
191-
ASSERT_NE(new_thread_local_plugin2->wasm(), old_wasm_ptr);
186+
ASSERT_NE(thread_local_plugin->wasm(), old_wasm_ptr);
192187

193188
// This time, create another thread local plugin with *different* plugin key for the same vm_key.
194189
// This one should reuse the recovered VM.
@@ -199,53 +194,53 @@ TEST_P(TestVm, RecoverCrashedThreadLocalWasm) {
199194
ASSERT_TRUE(thread_local_plugin_another && thread_local_plugin_another->plugin());
200195
ASSERT_FALSE(thread_local_plugin_another->wasm()->isFailed());
201196
// Verify the pointer to WasmBase is same with recovered one
202-
ASSERT_EQ(thread_local_plugin_another->wasm(), new_thread_local_plugin2->wasm());
197+
ASSERT_EQ(thread_local_plugin_another->wasm(), thread_local_plugin->wasm());
203198

199+
old_wasm_ptr = thread_local_plugin->wasm();
204200
// Cause runtime crash again.
205-
new_thread_local_plugin2->wasm()->wasm_vm()->fail(FailState::RuntimeError, "runtime error msg");
206-
ASSERT_TRUE(new_thread_local_plugin2->wasm()->isFailed());
201+
thread_local_plugin->wasm()->wasm_vm()->fail(FailState::RuntimeError, "runtime error msg");
202+
ASSERT_TRUE(thread_local_plugin->wasm()->isFailed());
207203
// Create another thread local plugin with *different* plugin key before recover.
208204
// This one also should not end up using the failed VM.
209205
auto thread_local_plugin_another2 = getOrCreateThreadLocalPlugin(
210206
base_wasm_handle, plugin_another, wasm_handle_clone_factory, plugin_handle_factory);
211207
ASSERT_TRUE(thread_local_plugin_another2 && thread_local_plugin_another2->plugin());
212208
ASSERT_FALSE(thread_local_plugin_another2->wasm()->isFailed());
213209
// Verify the pointer to WasmBase is different from the failed one.
214-
ASSERT_NE(thread_local_plugin_another2->wasm(), new_thread_local_plugin2->wasm());
210+
ASSERT_NE(thread_local_plugin_another2->wasm(), thread_local_plugin->wasm());
215211
// Do recover again.
216-
std::shared_ptr<PluginHandleBase> new_thread_local_plugin3;
217-
ASSERT_TRUE(new_thread_local_plugin2->doRecover(new_thread_local_plugin3));
218-
// Recover should reuse the VM already created by the new plugin
219-
ASSERT_EQ(new_thread_local_plugin3->wasm(), thread_local_plugin_another2->wasm());
212+
ASSERT_TRUE(thread_local_plugin->doRecover());
213+
// Verify the pointer to WasmBase is different from the crashed one.
214+
ASSERT_NE(thread_local_plugin->wasm(), old_wasm_ptr);
220215

216+
old_wasm_ptr = thread_local_plugin_another2->wasm();
221217
// Cause the another plugin with same vm_key crash.
222218
thread_local_plugin_another2->wasm()->wasm_vm()->fail(FailState::RuntimeError,
223219
"runtime error msg");
224220
ASSERT_TRUE(thread_local_plugin_another2->wasm()->isFailed());
225-
old_wasm_ptr = thread_local_plugin_another2->wasm();
226221
// Do recover again
227-
std::shared_ptr<PluginHandleBase> new_thread_local_plugin_another2;
228-
ASSERT_TRUE(thread_local_plugin_another2->doRecover(new_thread_local_plugin_another2));
229-
ASSERT_EQ(thread_local_plugin_another2->wasm(), new_thread_local_plugin_another2->wasm());
222+
ASSERT_TRUE(thread_local_plugin_another2->doRecover());
230223
// Verify the pointer to WasmBase is different from the crashed one.
231-
ASSERT_NE(new_thread_local_plugin_another2->wasm(), old_wasm_ptr);
224+
ASSERT_NE(thread_local_plugin_another2->wasm(), old_wasm_ptr);
232225

226+
old_wasm_ptr = thread_local_plugin_another2->wasm();
233227
// Cause the another plugin crash again
234-
new_thread_local_plugin_another2->wasm()->wasm_vm()->fail(FailState::RuntimeError,
235-
"runtime error msg");
236-
ASSERT_TRUE(new_thread_local_plugin_another2->wasm()->isFailed());
228+
thread_local_plugin_another2->wasm()->wasm_vm()->fail(FailState::RuntimeError,
229+
"runtime error msg");
230+
ASSERT_TRUE(thread_local_plugin_another2->wasm()->isFailed());
237231
// Create thread local plugin with same plugin key
238-
auto new_thread_local_plugin_another3 = getOrCreateThreadLocalPlugin(
232+
auto thread_local_plugin_another3 = getOrCreateThreadLocalPlugin(
239233
base_wasm_handle, plugin_another, wasm_handle_clone_factory, plugin_handle_factory);
240-
ASSERT_TRUE(new_thread_local_plugin_another3 && new_thread_local_plugin_another3->plugin());
241-
ASSERT_FALSE(new_thread_local_plugin_another3->wasm()->isFailed());
234+
ASSERT_TRUE(thread_local_plugin_another3 && thread_local_plugin_another3->plugin());
235+
ASSERT_FALSE(thread_local_plugin_another3->wasm()->isFailed());
242236
// Verify the pointer to WasmBase is different from the failed one.
243-
ASSERT_NE(new_thread_local_plugin_another3->wasm(), new_thread_local_plugin_another2->wasm());
237+
ASSERT_NE(thread_local_plugin_another3->wasm(), thread_local_plugin_another2->wasm());
244238
// Do recover again.
245-
std::shared_ptr<PluginHandleBase> new_thread_local_plugin_another4;
246-
ASSERT_TRUE(new_thread_local_plugin_another2->doRecover(new_thread_local_plugin_another4));
247-
// Recover should reuse the plugin handle
248-
ASSERT_EQ(new_thread_local_plugin_another4, new_thread_local_plugin_another3);
239+
ASSERT_TRUE(thread_local_plugin_another2->doRecover());
240+
// Recover should not reuse the plugin handle
241+
ASSERT_NE(thread_local_plugin_another2, thread_local_plugin_another3);
242+
// Recover shoud reuse the wasm handle
243+
ASSERT_EQ(thread_local_plugin_another2->wasm(), thread_local_plugin_another3->wasm());
249244
}
250245

251246
// Tests the canary is always applied when making a call `createWasm`

0 commit comments

Comments
 (0)