Fix panic on pollable drop - #706
WhiteAbeLincoln wants to merge 5 commits into
Conversation
90edfc0 to
fef4537
Compare
|
@harmony7 @cceckman-at-fastly I'd appreciate a review when you get time - no rush. I suggest you review commit by commit though since there's a bulk format of the test fixtures in there which will add a bunch of noise. This PR adds support for wasm32-wasip2 test fixtures (since afaik the issue doesn't occur under an adapted wasip1 component). That's loosely based off of wasmtime's test-programs crate with the addition of a manifest to control the architecture (and maybe other build flags in the future? One of the tests was failing when built under opt = 1). This is more complex than just listing the fixture separately in the Makefile, but I preferred it since it ensures the test fixtures get automatically built when they've changed and you re-run a test, and it means a The fix itself is in d6ef4ed. This is similar to the equivalent code in Compute & Charles's PR 600, though I was trying to avoid unnecessary breaking changes and so didn't add new variants to the AsyncItem enum. |
fef4537 to
25718c8
Compare
Test fixtures are now part of the main workspace instead of a separate crate, which means that CI fails because they're not formatted correctly. This is a one-off bulk reformat, CI will enforce future changes are correct.
Under the component ABI, if cache::Entry::step handed out a pollable and the pollable was later dropped before the entry instance was, subsequent use of the entry resulted in a panic because the async item had been dropped from the sandbox. The compute service doesn't have this issue because it checks the kind of associated AsyncItem in the sandbox when dropping a pollable. Some kinds are owned by the pollable handle, and some are owned by another resource, with the pollable acting as a weak handle. Porting this fix directly from the compute service would result in breaking changes in viceroy-lib's public API (though I don't know if this API is intended to be public or it's accidental). Additionally, adding more items to the `AsyncItem` enum would model types which aren't yet supported by Viceroy. Instead, a boolean flag is added to the `PendingCacheTask` struct to indicate whether this item is shared (and so can't be dropped through a pollable handle) or owned solely by the pollable.
25718c8 to
30bbf08
Compare
Under the component ABI, if cache::Entry::step handed out a pollable
and the pollable was later dropped before the entry instance was,
subsequent use of the entry resulted in a panic because the async item
had been dropped from the sandbox.
The compute service doesn't have this issue because it checks the kind
of associated AsyncItem in the sandbox when dropping a pollable. Some
kinds are owned by the pollable handle, and some are owned by another
resource, with the pollable acting as a weak handle.
Porting this fix directly from the compute service would result in
breaking changes in viceroy-lib's public API (though I don't know if
this API is intended to be public or it's accidental). Additionally,
adding more items to the
AsyncItemenum would model types which aren'tyet supported by Viceroy. Instead, a boolean flag is added to the
PendingCacheTaskstruct to indicate whether this item is shared (andso can't be dropped through a pollable handle) or owned solely by the
pollable.
Closes #696