Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The tests do not verify that a legacy //export function remains callable after _start returns with the scheduler enabled.
1 open issue
What changed in this PR
This PR enables wasi-legacy for programs using goroutines while preserving its return-from-_start behavior so the host can continue using the module.
Changes:
- Removes the scheduler restriction for
wasi-legacy. - Flushes C stdio when legacy
mainreturns, with no-op helpers for other WebAssembly environments. - Adds a default-scheduler test and checks that the legacy module remains open.
| File | Description |
|---|---|
src/runtime/runtime_wasmentry.go |
Flushes stdio after legacy main returns. |
src/runtime/runtime_wasm_unknown.go |
Adds a no-op stdio flush helper. |
src/runtime/runtime_tinygowasmp2.go |
Adds a no-op stdio flush helper for WASIp2. |
src/runtime/runtime_tinygowasm.go |
Implements flushing through __stdio_exit. |
main_test.go |
Tests legacy mode with the default scheduler and checks module state. |
builder/build.go |
Removes the scheduler requirement for legacy mode. |
Review effort: Balanced
|
@dgryski raised to me on Slack that panics will occur if a goroutine acts on its own after I had Claude run a bunch of different scenarios, here's the outcome:
We could make the panic error message better with a simple check, and that would be an improvement in all the modes where the runtime panics, not just |
Programs that start goroutines can now use -buildmode=wasi-legacy. Their _start returns when main returns instead of calling proc_exit, so the module stays open and the host can call the program's //export functions afterwards, which is what wasi-legacy was added for in tinygo-org#4734. Until now wasi-legacy required -scheduler=none, which cannot compile a go statement, so these programs could not use it at all. Returning from _start is also much cheaper than exiting. When proc_exit unwinds out of the module, wazero and wasmtime build a stack trace and symbolize it if debug info is enabled, which is the default in wazero and opt in for wasmtime. For a fresh instance of a 1.6MB program, wazero with its default config took 0.5ms instead of 3.6ms, and wasmtime with backtrace details on took 0.5ms instead of 11.5ms. Goroutines need no other change, because the wasi-legacy _start already runs the scheduler like the default _start does. The C stdio buffers are now flushed when main returns, as _start did before it started calling proc_exit.
TestWasmExport runs its export calls from inside main, so with wasi-legacy it could only check that the module stays open after _start. The new test builds a program whose main waits for a goroutine, runs _start, and then calls an //export function to read the value the goroutine set.
An //export function runs outside any goroutine, so with the Asyncify scheduler a blocking call there found no current task and failed with a nil pointer dereference. The blocking paths now get the current task from task.CurrentToBlock, which panics with "trying to do blocking operation in exported function" when there is none. Other schedulers return the current task as before, so their code size does not change. This happens in every build mode, and with -buildmode=wasi-legacy after main returns. //go:wasmexport functions run in a goroutine and can block.
343c6c6 to
7a4a59e
Compare
|
rebased on current dev to resolve conflicts |
davecheney
left a comment
davecheney
left a comment
There was a problem hiding this comment.
Thank you for working on this. I think its very close.
ISTM that this PR is bigger than it needs to be because of the renaming of task.Current. I know I added a bunch of naming suggestions, but that kind of underscores my point, the name of the function is the valuable part of this PR so maybe dropping, or deferring it, would avoid distracting the easily distracted such as myself.
| //go:linkname blockingPanic runtime.blockingPanic | ||
| func blockingPanic() | ||
|
|
||
| // CurrentToBlock returns the goroutine that the caller is about to block. It |
There was a problem hiding this comment.
s/goroutine/task
I'm not super sure that goroutine is a synonym for task or vice versa, but given the function returns a task, that naming makes more sense for me.
|
|
||
| // CurrentToBlock returns the goroutine that the caller is about to block. It | ||
| // panics when there is none, as in an //export function. | ||
| func CurrentToBlock() *Task { |
There was a problem hiding this comment.
suggestion: s/CurrentToBlock/CurrentTaskOrPanic
I'm a huge fan of Kate Gregory's suggestions that if the name of a function is good enough, it doesn't need a comment. I think this is an opportunity here.
|
|
||
| // Can't proceed. Add us to the list of senders and wait until we're awoken. | ||
| t := task.Current() | ||
| t := task.CurrentToBlock() |
There was a problem hiding this comment.
Modulo my comments above, given there is an implied task prefix, task.CurrentOrPanic is probably an even better name
Programs that start goroutines can now use
-buildmode=wasi-legacy. Their_startreturns when main returns instead of callingproc_exit, so the module stays open and the host can call the program's//exportfunctions afterwards, which is what wasi-legacy was added for in #4734. Until now wasi-legacy required-scheduler=none, which cannot compile a go statement, so these programs could not use it at all.Returning from
_startis also much cheaper than exiting. Whenproc_exitunwinds out of the module, wazero and wasmtime build a stack trace and symbolize it if debug info is enabled, which is the default in wazero and opt in for wasmtime. For a fresh instance of a 1.6MB program, wazero with its default config took 0.5ms instead of 3.6ms, and wasmtime with backtrace details on took 0.5ms instead of 11.5ms. On a production service (on wasmtime, with backtraces) this change alone cut more than 10ms off host time on each request.Goroutines need no other change, because the wasi-legacy
_startalready runs the scheduler like the default_startdoes. The C stdio buffers are now flushed when main returns, as_startdid before it started callingproc_exit.