feat(api): let embedders grow a shared memory without a store - #6877
feat(api): let embedders grow a shared memory without a store#6877Arshia001 wants to merge 5 commits into
Conversation
`SharedMemory` is already a clonable, `Send + Sync` handle that outlives
any particular store, but it exposed no way to grow the memory, so an
embedder holding one on a thread with no store could not claim pages.
wasmer-napi needs exactly that: its `GuestHeap` claims guest linear
memory from host code that runs on V8's GC threads.
Add three store-free methods, keeping the VM types encapsulated:
- `grow(&self, delta) -> Result<Pages, MemoryError>` -- the operation
itself. `&self` rather than `&mut self` because a shared memory is
an `Arc<RwLock<..>>` internally and any clone can already grow it;
pretending otherwise would be a lie the borrow checker can't keep.
- `data_ptr(&self) -> Option<*mut u8>` -- the same pointer
`MemoryView::data_ptr` already returns publicly, minus the store.
- `style(&self) -> Option<MemoryStyle>` -- so a caller can check that
the mapping is reserved up front before caching `data_ptr`.
`MemoryStyle` is already public API.
`data_ptr` matters because growth is not always base-stable:
`WasmMmap::grow` remaps a dynamic heap ("it's a dynamic heap and it can
move"). A caller that caches the base must first confirm a
`MemoryStyle::Static` bound covering the maximum, and re-check the base
after each grow -- both now possible without reaching into the VM layer.
Only the `sys` backend can do any of this store-free; v8 and js return
`MemoryError::UnsupportedOperation` / `None` rather than panicking.
Also bumps the napi submodule (which subsumes the previous bump-only
stack) and picks up `offset-allocator`/`nonmax` in Cargo.lock for the
GuestHeap allocator.
… handle Picks up wasmerio/napi#54, which replaces the private-VM-type accessor GuestHeap was using with the store-free SharedMemory::grow/data_ptr/style API added in wasmerio/wasmer#6877. Removes an unwrap_sys_ref() that would panic on the v8/js backends, and the last unsafe deref on the grow path.
There was a problem hiding this comment.
Pull request overview
This PR extends the public SharedMemory API to support store-free operations needed by embedders (notably growing shared linear memory and inspecting its host mapping) while keeping VM-layer types encapsulated, and folds in the associated dependency lockfile updates.
Changes:
- Add store-free
SharedMemory::{grow,data_ptr,style}APIs, returningUnsupportedOperation/Noneon non-sysbackends. - Add VM-internal helpers on
VMSharedMemoryto implement grow / base pointer / style queries per backend. - Update
Cargo.lockto includeoffset-allocatorandnonmaxentries (and wireoffset-allocatorinto the relevant package dependencies).
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/api/src/vm/impls.rs | Adds VMSharedMemory::{grow,data_ptr,style} with backend-specific behavior. |
| lib/api/src/entities/memory/shared.rs | Exposes new public store-free SharedMemory methods that delegate to VMSharedMemory. |
| Cargo.lock | Adds lock entries for nonmax / offset-allocator and updates dependency list accordingly. |
Suppressed comments (1)
lib/api/src/vm/impls.rs:117
- Same issue as the v8 branch: the
\continuation plus indentation inserts extra spaces into the rendered error message.
Self::Js(_) => Err(wasmer_types::MemoryError::UnsupportedOperation {
message: "growing a shared memory without a store is not supported by the js \
backend"
.into(),
}),
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// Grows this memory by `delta` pages, returning the previous size. | ||
| /// | ||
| /// Only the `sys` backend can grow a shared memory without a store; the | ||
| /// others report [`MemoryError::UnsupportedOperation`]. |
There was a problem hiding this comment.
Valid — fixed in b0931ee.
Reproduced with cargo doc -p wasmer --features sys --no-deps --document-private-items:
error: unresolved link to `MemoryError::UnsupportedOperation`
--> lib/api/src/vm/impls.rs:88:25
It slipped past my earlier plain cargo doc check because mod vm is private and rustdoc skips private modules — but make build-docs passes --document-private-items, so it would have bitten there. Now fully qualified as crate::MemoryError, which resolves in every configuration since wasmer-types is a mandatory dependency and the re-export at lib.rs is unconditional. Verified: zero impls.rs diagnostics under that flag now.
| pub fn grow(&self, delta: Pages) -> Result<Pages, MemoryError> { | ||
| self.memory.grow(delta) | ||
| } |
There was a problem hiding this comment.
Valid — added in b0931ee, three tests in a #[cfg(feature = "sys")] submodule that builds a shared Memory, detaches it with as_shared, and then:
grow_returns_previous_size_and_is_shared_across_clones—growreturns the size the memory had beforehand, the growth is visible through an independent clone of the handle (the whole point of the API being&self), and it refuses to exceed the declared maximum.data_ptr_is_available_and_stable_across_grow—data_ptrisSomeand non-null, and does not move across a grow whenstylereports aStaticbound covering the maximum. That is the invariant that makes caching the base sound, and it is exactly what wasmer-napi'sGuestHeaprelies on, so it is worth pinning here rather than only downstream.style_is_reported—styleisSomeon the sys backend.
4/4 green (including the pre-existing Send/Sync test).
There was a problem hiding this comment.
I need to revisit this fully. I'm not convinced to not take one store handle for it at least. But it may work. I'm concerning of having a path that will work for sys, but not v8.
There was a problem hiding this comment.
TBH, me too. I'm going back to this, seeing if we can't come up with something cleaner.
…ests
Two review findings on the store-free `SharedMemory` API:
- The `[`MemoryError::UnsupportedOperation`]` link in `vm/impls.rs` does
not resolve: `MemoryError` is not in scope there. It slipped through
a plain `cargo doc` because `mod vm` is private and rustdoc skips
private modules, but `make build-docs` passes
`--document-private-items`, where it is a hard error under the
crate's `#![deny(rustdoc::broken_intra_doc_links)]`. Fully qualify it
as `crate::MemoryError`, which resolves in every configuration since
`wasmer-types` is a mandatory dependency and the re-export is
unconditional.
- `grow`/`data_ptr`/`style` had no tests. Add three on the sys backend:
`grow` reports the previous size, is visible through an independent
clone of the handle, and refuses to exceed the declared maximum;
`data_ptr` is non-null and does not move across a grow when the style
reports a `Static` bound covering the maximum (the invariant that
makes caching the base sound); and `style` is reported at all.
The new store-free tests were gated on `cfg(feature = "sys")` and asserted
that `grow`/`data_ptr`/`style` all succeed. That gate is wrong: the
workspace test matrix builds `wasmer` with `sys` compiled in (feature
unification across `--workspace`) while `Store::default()` resolves to a
different backend entirely. On `All tests - windows-x64`, which runs
`--no-default-features --features v8,v8-default`, the tests compiled and
then failed against a v8-backed memory:
grow: UnsupportedOperation { message: "growing a shared memory
without a store is not supported by the v8 backend" }
Assert the contract each backend actually implements instead. The three
accessors are all-or-nothing, so the test branches on whether the set is
available and checks the corresponding half — full grow/base-stability
behaviour where it is, and a graceful `UnsupportedOperation` (rather than
a panic) where it is not. It also pins that the three agree with each
other: a backend offering `data_ptr` without `style` would leave a caller
unable to tell whether caching the base is sound.
Verified both halves do real work rather than skipping: with
`sys,cranelift` the test takes the supported branch, and with
`sys,headless,v8,v8-default` (the shape that failed on Windows) it takes
the unsupported one.
…mand
When a PATH search finds nothing, proc_spawn3 returned Errno::Noexec.
POSIX reserves ENOEXEC for a file that exists but is not an executable
format; "no such command" is ENOENT. proc_exec4 already returns Noent
from the identical match, so the two syscalls disagreed about the same
condition.
This is not cosmetic. Node's child_process only converts a spawn failure
into an asynchronous 'error' event for a known set of errnos:
if (err === UV_EACCES || err === UV_EAGAIN || err === UV_EMFILE ||
err === UV_ENFILE || err === UV_ENOENT) {
process.nextTick(onErrorNT, this, err);
} else if (err) {
throw new ErrnoException(err, 'spawn');
}
ENOEXEC is not in that set, so a missing external command threw out of
child_process.spawn() instead of reaching the caller's callback. Code
written to degrade when an optional tool is absent could not: the throw
bypasses the very handler meant to catch it. Node's own
test-http-full-response, which calls printSkipMessage('problem spawning
`ab`') when ApacheBench is unavailable, died with an uncaught
Error: spawn ENOEXEC
at ChildProcess.spawn (node:internal/child_process:421:11)
on the edgejs WASIX lane rather than skipping. With ENOENT the error is
delivered to the callback and the fallback path runs.
The Noexec returned further down, after the executable resolves but the
spawn itself fails, is left alone: there the name did resolve, so
ENOEXEC is the accurate answer.
child_process.exec() and execSync() do not spawn the command directly -- they run it through `/bin/sh -c`. The root package ships no shell, so every exec() in the V8 WASIX package failed to spawn one. The QuickJS package has declared this dependency since it was created; this one never did. Surfaced by test-http-chunk-problem and test-http-full-response on the v8-wasix lane, which #115 stops skipping. Both exec an external tool and both are written to cope with it being absent, but they never got the chance: with no shell to resolve, the spawn failed before their callback ran. They pass on quickjs-wasix, which has the shell. The uncaught throw those tests hit is a second, separate bug -- proc_spawn reported ENOEXEC rather than ENOENT for an unresolvable command, and Node only converts a known errno set into an 'error' event (wasmerio/wasmer#6877). Either fix alone makes these two tests pass; both are worth having, since this one also makes exec() work at all.
…stance A process that has installed a signal handler could still be killed by the runtime's default disposition, because the "has a handler" flag lives on InstanceHandles and every spawned thread gets its own instance. callback_signal() sets it on whichever instance registered, so the main thread knows about the handler and the sibling threads do not. Signals are delivered to every thread of the process. When one of the siblings drains its pending queue, process_signals_and_exit() sees `!inner.signal_set`, concludes the program handles no signals, and applies the default disposition -- terminating the whole process even though the handler is installed and the main thread is dispatching to it correctly. Record the registration on WasiState, which is shared by the threads of a process, and consult that in addition to the per-instance flag. Observed as a broken-pipe write killing edgejs. fd_write() raises SIGPIPE alongside returning EPIPE, so a write to a dead child's stdin terminated the process with exit code 64 (Errno::Pipe) rather than surfacing EPIPE to the guest. Node installs a SIGPIPE disposition precisely so that writes report EPIPE instead, and its callback was registered -- but on the main instance only, so a worker thread applied the default and exited. Traced to: [thread A] fd_write -> processing signal via handler -> returns Errno::pipe [thread B] default-disposition terminate sig=Sigpipe exit=64 signal_set=false The forked-state copies start out false: a forked or spawned program re-registers from its own instance and should not inherit the parent's. Verified on the edgejs WASIX lane with `serve` (whose clipboardy dependency shells out to a `xsel` binary that does not exist in the guest): the process previously died with exit 64 and now stays up, reporting the same ENOENT that Linux does. Full safe-mode suite passes, including "SIGINT without a JS listener terminates" and "SIGTERM reaches a JS listener", and node:dgram + node:tls + node:http run 644 passed / 0 failed.
Replaces the whole
chore: bump napistack (#6819, #6820, #6821, #6822, #6823, #6825, #6829, #6844, #6845, #6850) with a single PR. Nine of those ten were purelib/napisubmodule bumps; the napi pin here is a descendant of every one of them, so this subsumes the lot. The tenth, #6822, is the only one that carried code, and it is reworked below.The problem with #6822
#6822 added
SharedMemory::vm_shared_memory() -> &VMSharedMemoryso wasmer-napi'sGuestHeapcould grow guest linear memory from host code running on V8's GC threads. That accessor was the wrong seam:mod vminlib/api/src/lib.rsis private and re-exported nowhere, sowasmer::vm::VMSharedMemoryhas no public path. The consumer had to write one unbreakable expression,shared.vm_shared_memory().unwrap_sys_ref().clone(), because the intermediate can never be bound.unwrap_sys_ref()panics on a v8 or js memory — a "sys backend only" contract expressed nowhere in the type.cargo docoff thesysfeature. The doc comment linkedwasmer_vm::LinearMemory::grow, butwasmer-vmis optional and the crate has#![deny(rustdoc::broken_intra_doc_links)], socargo doc --no-default-features --features v8failed. (Copilot flagged this on GuestHeap: SharedMemory accessor + napi bump + Cargo.lock (ECO-416) #6822; reproduced, and it was the only error in that build.) The mismatch was a symptom: the docs described the sys type while the signature returned the backend enum.SharedMemoryis a clonable,Send + Sync, store-free handle — there's a test asserting it. The actual gap was that it had no store-freegrow.This PR instead
Three store-free methods on
SharedMemory, all keeping the VM types encapsulated:grow(&self, delta) -> Result<Pages, MemoryError>&self, not&mut self: a shared memory is anArc<RwLock<..>>internally and any clone can already grow it, so&mutwould be a distinction the type can't defenddata_ptr(&self) -> Option<*mut u8>MemoryView::data_ptralready returns publicly, minus the storestyle(&self) -> Option<MemoryStyle>MemoryStyleis already public API; lets a caller confirm the mapping is reserved up front before cachingdata_ptrdata_ptrandstyleare needed together because growth is not always base-stable —WasmMmap::growremaps a dynamic heap ("it's a dynamic heap and it can move"). A caller caching the base must confirm aMemoryStyle::Staticbound covering the maximum and re-check the base after each grow. Both are now possible without reaching into the VM layer; previously the second required an unsafe deref ofVMMemoryDefinition.Only
syssupports any of this store-free. v8 and js returnMemoryError::UnsupportedOperation/Noneinstead of panicking.Also carries the
offset-allocator/nonmaxCargo.lockentries that #6822 introduced for the GuestHeap allocator.Testing
cargo check -p wasmer --features sys— green.cargo check -p wasmer --no-default-features --features v8— green.cargo doc -p wasmer --no-default-features --features v8 --no-deps— green (fails on GuestHeap: SharedMemory accessor + napi bump + Cargo.lock (ECO-416) #6822).cargo test -p wasmer --features sys --lib— the existingensure_shared_memory_handles_are_send_and_synctest passes.cargo test -p wasmer-napi --lib— 38/38.napi-v8CLI built from this branch: payload load test, 20 rounds / 3.91 GiB verified byte-for-byte, per-round retention flat.Paired napi change: wasmerio/napi#54 (stacked on wasmerio/napi#53). The submodule here points at #54's head, so #53 and #54 need to merge first.