Add timeout support in wazero run cli
#1173
Conversation
de39257 to
b60d901
Compare
cmd/wazero/wazero.go
Outdated
| } | ||
|
|
||
| if err != nil { | ||
| if exitErr, ok := err.(*sys.ExitError); ok { | ||
| exit(int(exitErr.ExitCode())) | ||
| exitCode := exitErr.ExitCode() | ||
| if timeout > 0 && exitCode == sys.ExitCodeDeadlineExceeded { |
There was a problem hiding this comment.
do we need timeout >0? I guess sys.ExitCodeDeadlineExceeded won't be set if timeout == 0
cmd/wazero/wazero.go
Outdated
| exit(int(exitErr.ExitCode())) | ||
| exitCode := exitErr.ExitCode() | ||
| if timeout > 0 && exitCode == sys.ExitCodeDeadlineExceeded { | ||
| fmt.Fprintf(stdErr, "canceled: wasm binary '%s' exceeded timeout (%v)\n", wasmExe, timeout) |
There was a problem hiding this comment.
can we just reuse exitErr.Error() https://github.com/tetratelabs/wazero/blob/main/sys/error.go#L59-L68 ?
There was a problem hiding this comment.
done, although the error becomes slightly more cryptic:
error: module "" closed with context deadline exceeded (timeout 1ms)
I have still added the timeout
cmd/wazero/wazero.go
Outdated
| @@ -133,6 +134,11 @@ func doRun(args []string, stdOut io.Writer, stdErr logging.Writer, exit func(cod | |||
| "This may be specified multiple times. When <wasm path> is unset, <path> is used. "+ | |||
| "For read-only mounts, append the suffix ':ro'.") | |||
|
|
|||
| var timeout time.Duration | |||
| flags.DurationVar(&timeout, "timeout", 0*time.Second, | |||
| "if a wasm binary runs longer than the given duration, then panic. "+ | |||
There was a problem hiding this comment.
for non gophers, the duration format for this string might not be common. Maybe add example here??
There was a problem hiding this comment.
the message is tad longer now, but it should be good; basically I lifted the one in the Go docs for ParseDuration except the note about it being negative because it makes no sense: I also added a test to return an error in case of a negative value
|
also for another PR I think we should test ctrl-c (sigint) on an endless loop. Not sure, but maybe we'll end up needing context integration and a signal handle.. |
- ensure the module initialization function is evaluated regardless if it's WASI or GoJS - add a `-timeout duration` flag - ensure flag gets propagated to the rt config builder (also ensure cache flag gets propagated to the rt config builder) - print a message to stderr when the deadline is exceeded - configure GitHub Actions to use `-timeout=10m` flag instead of GHA's `timeout-minutes: 10` Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>
b60d901 to
7e4d9fb
Compare
| c = c.WithCompilationCache(cache) | ||
| } | ||
|
|
||
| ctx := context.Background() | ||
| rt := wazero.NewRuntime(ctx) | ||
| rt := wazero.NewRuntimeWithConfig(ctx, c) |
There was a problem hiding this comment.
we forgot to use c here
Unless I misunderstood, I think it already works? 🤔 c-c works as expected (no change required) |
|
sorry about ctrl-c I didn't know if it did or not (we should backfill a test. there's a similar one in func-e under the e2e directory). anyway different but similar topic though out of scope here! |
regardless if it's WASI or GoJS
-timeout durationflag(also ensure cache flag gets propagated to the rt config builder)
-timeout=10mflaginstead of GHA's
timeout-minutes: 10Signed-off-by: Edoardo Vacchi evacchi@users.noreply.github.com