add missing may-block checks for sync-to-sync guest-to-guest calls #12282
Conversation
github-actions
bot
added
the
wasmtime:api
Previously, we weren't updating or checking the may-block status of a task across sync-to-sync, guest-to-guest calls, meaning we were allowing blocking in cases we shouldn't have. This fixes that by adding a new `task_may_block` field to `VMComponentContext`, plus code to update it every time we switch threads or do a sync-to-sync, guest-to-guest call. We use that field as the source of truth about whether a blocking operation is permitted. I've updated various tests to match, and Luke has an item on his to-do list to add sad-path coverage for various cases to the upstream `component-model` test suite.
dicej
force-pushed
the
better-may-block-tracking
branch
from
4954f10 to
a64f444
Compare
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Test-wise, to confirm, you feel that the upstream spec tests adequately cover this? And/or the test updates that were required here? Or should some dedicated tests be written as well?
| #[repr(transparent)] | ||
| #[derive(Copy, Clone)] | ||
| pub struct TaskMayBlock(SendSyncPtr<VMGlobalDefinition>); | ||
|
|
||
| impl TaskMayBlock { | ||
| pub unsafe fn get(&self) -> bool { | ||
| unsafe { *self.as_raw().as_ref().as_i32() != 0 } | ||
| } | ||
|
|
||
| pub unsafe fn set(&mut self, val: bool) { | ||
| unsafe { | ||
| *self.as_raw().as_mut().as_i32_mut() = if val { 1 } else { 0 }; | ||
| } | ||
| } | ||
|
|
||
| pub fn as_raw(&self) -> NonNull<VMGlobalDefinition> { | ||
| self.0.as_non_null() | ||
| } | ||
| } |
There was a problem hiding this comment.
The old InstanceFlags structure is pretty unsafe due to retaining a long-lived pointer disconnected from the ComponentInstance, so ideally we wouldn't model this after that if we don't need to. Would it be possible to have safe get/set/as_raw methods on ComponentInstance and avoid having this type entirely?
dicej
force-pushed
the
better-may-block-tracking
branch
from
437e308 to
3bc1908
Compare
dicej
force-pushed
the
better-may-block-tracking
branch
from
cdc79e1 to
b1d92e1
Compare
(Sorry, I forgot to respond to this earlier) We do need dedicated tests for this, and I've added an item to @lukewagner's "WAST-as-a-service" backlog to cover that. |
|
Makes sense, but while that makes sense for retroactively testing things I'd prefer to try to ensure that we are also writing |
2 participants
Previously, we weren't updating or checking the may-block status of a task across sync-to-sync, guest-to-guest calls, meaning we were allowing blocking in cases we shouldn't have.
This fixes that by adding a new
task_may_blockfield toVMComponentContext, plus code to update it every time we switch threads or do a sync-to-sync, guest-to-guest call. We use that field as the source of truth about whether a blocking operation is permitted.I've updated various tests to match, and Luke has an item on his to-do list to add sad-path coverage for various cases to the upstream
component-modeltest suite.