Skip to content

Add terminate execution API. #268

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 12 commits into from
Mar 1, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion include/proxy-wasm/wasm.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,6 @@ class WasmBase : public std::enable_shared_from_this<WasmBase> {

void timerReady(uint32_t root_context_id);
void queueReady(uint32_t root_context_id, uint32_t token);

void startShutdown(std::string_view plugin_key);
void startShutdown();
WasmResult done(ContextBase *root_context);
Expand Down
1 change: 1 addition & 0 deletions include/proxy-wasm/wasm_vm.h
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,7 @@ class WasmVm {

// Integrator operations.
std::unique_ptr<WasmVmIntegration> &integration() { return integration_; }
virtual void terminateExecution() {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Could you move it higher? Right now, it's between 2 "integration" functions. Perhaps before isFailed()?
  2. I think using terminate as a name should be enough.
  3. Please mark it as a pure virtual function.
  4. Some documentation would be useful.

bool cmpLogLevel(proxy_wasm::LogLevel level) { return integration_->getLogLevel() <= level; }

protected:
Expand Down
10 changes: 10 additions & 0 deletions src/v8/v8.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,14 @@
#include <mutex>
#include <optional>
#include <sstream>
#include <thread>
#include <unordered_map>
#include <utility>
#include <vector>

#include "include/v8-version.h"
#include "include/v8.h"
#include "src/wasm/c-api.h"
#include "wasm-api/wasm.hh"

namespace proxy_wasm {
Expand Down Expand Up @@ -66,6 +68,14 @@ class V8 : public WasmVm {
const std::unordered_map<uint32_t, std::string> &function_names) override;
std::string_view getPrecompiledSectionName() override;
bool link(std::string_view debug_name) override;
void terminateExecution() override {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split declaration and implementation, like other functions in the code?

Also, the placement should match that in the wasm_vm.h.

auto *store_impl = reinterpret_cast<wasm::StoreImpl *>(store_.get());
auto *isolate = store_impl->isolate();
isolate->TerminateExecution();
while (isolate->IsExecutionTerminating()) {
std::this_thread::yield();
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we log this event? e.g.

Suggested change
}
integration()->trace("[host->vm] Terminated");
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, we should probably mark the VM as failed, so that other code paths won't try to continue execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we don't mark it as failed, the user can may reuse it. If we mark it as fail explicitly, we don't need to wait for the termination to finish.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think WasmVM is going to be reusable after we terminate it in the middle of the call. Risking resuming it in a weird state doesn't seem to be worth the trouble.

Also, I think we might be already marking it as failed, since the terminate call is going to happen during a callback, so it's going to results in a trap, which marks WasmVM as failed... but perhaps it makes sense to mark it explicitly to cover any future non-callback cases?

As for waiting for the termination to finish, I'd prefer to leave it in. It seems to be always instant in my tests (i.e. IsExecutionTerminating() is never true), and this way we can be sure that the WasmVM not busy anymore, so that vm->terminate(); delete vm; won't result in a crash.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that fail() is not thread safe, but terminate() should be, so I get a tsan error. Do you have any suggestion?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's drop explicit fail() call for now, since it's going to turn into a trap and failed WasmVM anyway when unrolling.


Cloneable cloneable() override { return Cloneable::CompiledBytecode; }
std::unique_ptr<WasmVm> clone() override;
Expand Down
36 changes: 36 additions & 0 deletions test/runtime_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@

#include "gtest/gtest.h"

#include <cassert>
#include <fstream>
#include <iostream>
#include <memory>
#include <sstream>
#include <string>
#include <thread>
#include <vector>

#include "include/proxy-wasm/context.h"
Expand Down Expand Up @@ -245,6 +247,40 @@ TEST_P(TestVM, Callback) {
ASSERT_EQ(res.u32(), 100100); // 10000 (global) + 100(in callback)
}

TEST_P(TestVM, TerminateExecution) {
if (engine_ != "v8") {
return;
}
auto source = readTestWasmFile("callback.wasm");
ASSERT_TRUE(vm_->load(source, {}, {}));

TestContext context;
vm_->registerCallback(
"env", "callback", &callback,
&ConvertFunctionWordToUint32<decltype(callback), callback>::convertFunctionWordToUint32);

vm_->registerCallback(
"env", "callback2", &callback2,
&ConvertFunctionWordToUint32<decltype(callback2), callback2>::convertFunctionWordToUint32);

std::thread terminate([&]() {
std::this_thread::sleep_for(std::chrono::seconds(3));
vm_->terminateExecution();
});

ASSERT_TRUE(vm_->link(""));
WasmCallWord<1> run2;
vm_->getFunction("infinite_loop", &run2);
EXPECT_TRUE(run2 != nullptr);

run2(&context, Word{0});
terminate.join();

std::string exp_message = "Function: infinite_loop failed: Uncaught Error: termination_exception";
auto *integration = dynamic_cast<DummyIntegration *>(vm_->integration().get());
ASSERT_TRUE(integration->error_message_.find(exp_message) != std::string::npos);
}

TEST_P(TestVM, Trap) {
auto source = readTestWasmFile("trap.wasm");
ASSERT_TRUE(vm_->load(source, {}, {}));
Expand Down
17 changes: 17 additions & 0 deletions test/test_data/callback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,23 @@ pub extern "C" fn run2(val: i32) -> i32 {
}
}

#[no_mangle]
pub extern "C" fn infinite_loop(val: i32) -> i32 {
let mut count = val;
loop {
count += 1;
if count >= 10 {
count -= 2;
}
if count >= 100 {
break;
}
}
unsafe {
callback2(val) + A
}
}

extern "C" {
fn callback();
fn callback2(val: i32) -> i32;
Expand Down