Consider integration of go's cancel context

[codefromthecrypt]

from @sam-at-luther on #421

From the go docs, it's not immediately clear if canceling the passed ctx will stop execution. From this discussion I gather that it does not (I have not tried it), and that the ctx is more for tracing purposes--is that right?

Indeed a cancel context only works if you pay attention to it.

Ex. reverseproxy.go in the main Go codebase guards its while loop on canceled, so it should stop abrupt if canceled.

    for {
        nr, rerr := src.Read(buf)
        if rerr != nil && rerr != io.EOF && rerr != context.Canceled {
            p.logf("httputil: ReverseProxy read error during body copy: %v", rerr)
        }

Most code I've seen in go doesn't pay attention to context.Canceled and we don't yet. There are some places it might be handy, ex in bulk memory (if really big) or a large I/O in WASI. It could also be the case in some loop forms we will need something like this to stop it from going forever, or some other mechanism to cancel (signal etc)

[codefromthecrypt]

PS for this to work, we need to default to a cancelcontext or wrap any incoming context in a cancelcontext. There's some expense to doing that as you could imagine that could cause overhead and/or allocation each time a host function is called. unlike context.Background a cancelcontext is stateful so defaulting to that has some expense to it.

[codefromthecrypt]

PS I noticed that go's source rarely guards on ctx.Err() even if blocking on ctx.Done() is frequent. Probably need to think through why this isn't used much inside Go itself, before suggesting things like guarding on ctx.Err() == nil prior to invoking I/O, especially as go's I/O isn't context aware anyway. Maybe no one checks ctx.Err() because it is an edge case and not much point if using non-contextualized APIs?

[robbertvanginkel]

I ran into something similar, where the context that I used for running a wasi binary had a timeout but the goroutine that was executing it with wazero seemed to hang indefinitely.

While I'm still tracking down exactly what was going in the particular wasm file, I a simple infinite loop wasi binary shows that the context's timeout/cancel doesn't stop/fail the execution:

package timeout

import (
    "context"
    _ "embed"
    "testing"
    "time"

    "github.com/stretchr/testify/assert"
    "github.com/tetratelabs/wazero"
    "github.com/tetratelabs/wazero/imports/wasi_snapshot_preview1"
    "github.com/tetratelabs/wazero/sys"
)

// infinite.wat:
//
//  (module
//    (import "wasi_snapshot_preview1" "proc_exit" (func $exit (param i32)))
//
//    (memory 1)
//    (export "memory" (memory 0))
//
//    (func $main (export "_start")
//      ;; infinite loop
//      (loop $l br $l)
//
//      (call $exit (i32.const 0))
//    )
//  )
//
// compile using wat2wasm
//go:embed infinite.wasm
var infiniteWasm []byte

func TestTimeout(t *testing.T) {
    ctx := context.Background()
    r := wazero.NewRuntime(ctx)
    defer r.Close(ctx) // This closes everything this Runtime created.

    timeoutCtx, cancel := context.WithTimeout(ctx, time.Second)
    defer cancel()
    wasi_snapshot_preview1.MustInstantiate(timeoutCtx, r)
    _, err := r.InstantiateModuleFromBinary(timeoutCtx, infiniteWasm)
    if exitErr, ok := err.(*sys.ExitError); ok {
        assert.Equal(t, uint32(0), exitErr.ExitCode())
    }
}

Using v1.0.0-pre.7, under the compiler on both arm64 and amd64, this fails like:

$ go test . -timeout=5s
panic: test timed out after 5s

From my look at the api surface there doesn't seem to be another way to bound or cancel the execution time of a wasm module in wazero yet? Other engines like wasmtime seem to have a --wasm-timeout flag, with which the wasm file above gets aborted.

[codefromthecrypt]

We haven't integrated anything, yet, to stop on a timeout or cancel context. ITOW, this issue is about the latter, which hasn't yet been implemented. There have been some chatter on #wazero on gophers slack recently, and I suspect some solution, or a way to implement one, will be made possible.

cc @mathetake for a summary comment on this specific one if you have one (integration of cancel context)

[robbertvanginkel]

You're right, deadline exceeded/context cancelled aren't the same so should perhaps have filed a separate issue rather than tagging along here. Please split it out or tell me to do so if that helps for your tracking.

I mostly wanted to highlight that the current behaviour caught me a bit off guard, and add a voice to this issue that hadn't seen some updates in a while that being able to limit wasi execution is something I'm interested in. If this is already being discussed on the slack though I'll peek in and wait for updates in the meantime!