Make ProcExit call Module.Close() #405
Conversation
nullpo-head
force-pushed
the
close_on_proc_exit
branch
from
5c4a369 to
576d107
Compare
Signed-off-by: Takaya Saeki <takaya@tetrate.io>
nullpo-head
force-pushed
the
close_on_proc_exit
branch
from
576d107 to
617dd9c
Compare
| sys, err := newSysContext(nil, nil, nil) | ||
| require.NoError(t, err) | ||
|
|
||
| _, store, mod, fn := instantiateModule(t, context.Background(), FunctionProcExit, ImportProcExit, moduleName, sys) |
There was a problem hiding this comment.
AFAICT, the signature change to return a 4th result is only used here. can you revert it and instantiate this test case different way? That prevents drifting almost all tests in this file.
There was a problem hiding this comment.
So, I would roll back changes to wasi_test.go.
There are two sorts of tests going on. wasi_test is unit tests and shouldn't do more than that. Anything more heavy should be tested in ad_hoc test so that multiple engines are used. testAdhocCloseWhileExecution does almost exactly what you were looking for and so heavier tests should be slight modifications to that.
For unit tests, there is little to do here, so should be a lot less code change. There's only one thing new going on. module.Close() is being invoked. You only need to test this was called, and that you are ignoring its errors (since the code added ignores errors).
So, for a test like that you need two instance of wasm.Module: one that succeeds on close and one that fails on close. You also need to be able to detect if it has closed.
You can instantiate the Module any way you like, including literally instantiating it. This means you can avoid heavy changes to the file to test these two cases.
Remember, WASI is only used when importing, so the importing module is the context used, you control which module instance is used when you use fn.Call
Hope these points help!
|
|
||
| require.Nil(t, store.Module(moduleName)) // verify moduleName was removed because ProcExit calls Close | ||
| }) | ||
| t.Run("close is called after proc_exit", func(t *testing.T) { |
There was a problem hiding this comment.
If we prove we called Module.Close(), the this test is unnecessary unless we've not tested Module.Close() is safe to call twice. So, we are missing a test file module_context_test.go which can be added in another PR to say it is safe to close twice. We already test what it doesn't internally via TestStore_CloseModule (this tests twice is fine)
| procExitFns := []struct { | ||
| name string | ||
| procExit func() (procExitFn, *wasm.Store) | ||
| }{ |
There was a problem hiding this comment.
these tests should be in adhoc.go or equivalent so that they go against both engines. testAdhocCloseWhileExecution has some overlap with this, so maybe change the test that does this to use WASI? or perhaps add another test dimension to do the same with WASI also. In any case I would remove the import and back stuff as that's more about the engine.
"close_module": func() { _ = moduleCloser() }, // Closing while executing itself.
|
Your points totally make sense and I'm working on fixing. This PR is not closed but will be completely rebased after it. |
|
Closing in favor of the different changes |
This PR makes ProcExit call
Module.Close()when it's called.proc_exitis expected to make the caller no longer usable, which means wazero can callModule.Close()and can release its resources. There may be further discussion that we can close the entire store, but closing module would be the good starting point anyway.Signed-off-by: Takaya Saeki takaya@tetrate.io