bytecodealliance / lucet

Lucet, the Sandboxing WebAssembly Compiler.
Apache License 2.0
4.06k stars 165 forks source link

replace `terminate_on_heap_oom` with wasmtime-like `MemoryLimiter` #682

Closed pchickey closed 3 years ago

pchickey commented 3 years ago

Lucet's Instance::terminate_on_heap_oom behavior was, from the beginning (https://github.com/bytecodealliance/lucet/pull/583), something that deviated from the Wasm spec. This PR eliminates it in favor of a different mechanism, MemoryLimiter. MemoryLimiter::memory_growing provides users a mechanism to allow or disallow a memory.grow operation, as well as await on memory becoming available. terminate_on_heap_ppm is replaced by the MemoryLimiter::memory_grow_failed method, where an unsuccessful grow (whether due to memory_growing, Limits, or an OS memory allocation failure) is reported to the embedder.

When we went to add heap-out-of-memory detection in Wasmtime, we had to figure out a way to do so without breaking the spec, and that turned into ResourceLimiter.

Here in Lucet, I introduced MemoryLimiter, which only does the memory-related operations of wasmtime's ResourceLimiter.

Unlike in Wasmtime, where we went to some pains to make it work with both sync and async embeddings, this embedding only works for async, because that is what Fastly's embedding needs and we aren't putting energy into generalizing the features.

Also unlike in Wasmtime, the MemoryLimiter doesn't attempt to restrict memory operations by the embedding API (e.g. Instance::grow_memory), because we don't end up needing it in the Fastly embedding. The MemoryLimiter only works on wasm memory.grow operations.

acfoltzer commented 3 years ago

Okay, just to make sure I understand, we will no longer get a runtime error back from Instance::run_async when the heap is exhausted. Instead, we'll set up some mechanism in a MemoryLimiter impl that keeps track of whether the heap has been exhausted, and then check+translate that into an embedder-specific error after the run completes?

pchickey commented 3 years ago

Yes, exactly that.