Add terminate execution API. #268
Conversation
Signed-off-by: chaoqin-li1123 <[email protected]>
chaoqin-li1123
requested review from
PiotrSikora and
mathetake
as code owners
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
include/proxy-wasm/wasm_vm.h
Outdated
| @@ -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.
- Could you move it higher? Right now, it's between 2 "integration" functions. Perhaps before
isFailed()? - I think using
terminateas a name should be enough. - Please mark it as a pure virtual function.
- Some documentation would be useful.
src/v8/v8.cc
Outdated
| @@ -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.
Could you split declaration and implementation, like other functions in the code?
Also, the placement should match that in the wasm_vm.h.
src/v8/v8.cc
Outdated
| while (isolate->IsExecutionTerminating()) { | ||
| std::this_thread::yield(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Should we log this event? e.g.
| } | |
| integration()->trace("[host->vm] Terminated"); | |
| } |
There was a problem hiding this comment.
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.
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.
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.
There was a problem hiding this comment.
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.
Let's drop explicit fail() call for now, since it's going to turn into a trap and failed WasmVM anyway when unrolling.
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
Signed-off-by: chaoqin-li1123 <[email protected]>
|
./retest |
|
/retest |
|
There is no |
Signed-off-by: chaoqin-li1123 <[email protected]>
Enable v8 wasm vm to terminate execution.
Signed-off-by: chaoqin-li1123 [email protected]