-
Notifications
You must be signed in to change notification settings - Fork 73
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
Changes from 4 commits
edc4684
2977924
1d5eeb0
31d7959
f0c6763
cd09203
1424878
c96cebd
c5a24cf
6d9e333
0cd4157
831fcee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -311,6 +311,7 @@ class WasmVm { | |
|
||
// Integrator operations. | ||
std::unique_ptr<WasmVmIntegration> &integration() { return integration_; } | ||
virtual void terminateExecution() {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
bool cmpLogLevel(proxy_wasm::LogLevel level) { return integration_->getLogLevel() <= level; } | ||
|
||
protected: | ||
|
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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 { | ||||||||
|
@@ -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 { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||||
auto *store_impl = reinterpret_cast<wasm::StoreImpl *>(store_.get()); | ||||||||
auto *isolate = store_impl->isolate(); | ||||||||
isolate->TerminateExecution(); | ||||||||
while (isolate->IsExecutionTerminating()) { | ||||||||
std::this_thread::yield(); | ||||||||
} | ||||||||
} | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we log this event? e.g.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's drop explicit |
||||||||
|
||||||||
Cloneable cloneable() override { return Cloneable::CompiledBytecode; } | ||||||||
std::unique_ptr<WasmVm> clone() override; | ||||||||
|
Uh oh!
There was an error while loading. Please reload this page.