Repository: tetratelabs/wazero
Pull request #418 (2022-03-30 02:20:43)

Adds Module.Name and ports all hammer tests to the same impl
This exposes `wasm.Module.Name()` so that it is more coherent to
redefine a module with the same name, or know the name to import.

This uses that to port existing tests to use the new hammer
infrastructure (without module name collision).



Patch:
diff --git a/internal/wasm/module_context.go b/internal/wasm/module_context.go
index ae247bd4..228d3e28 100644
--- a/internal/wasm/module_context.go
+++ b/internal/wasm/module_context.go
@@ -28,6 +28,11 @@ type ModuleContext struct {
 	sys *SysContext
 }
 
+// Name implements the same method as documented on wasm.Module
+func (m *ModuleContext) Name() string {
+	return m.module.Name
+}
+
 // WithMemory allows overriding memory without re-allocation when the result would be the same.
 func (m *ModuleContext) WithMemory(memory *MemoryInstance) *ModuleContext {
 	if memory != nil && memory != m.memory { // only re-allocate if it will change the effective memory
@@ -36,12 +41,12 @@ func (m *ModuleContext) WithMemory(memory *MemoryInstance) *ModuleContext {
 	return m
 }
 
-// String implements fmt.Stringer
+// String implements the same method as documented on wasm.Module
 func (m *ModuleContext) String() string {
-	return fmt.Sprintf("Module[%s]", m.module.Name)
+	return fmt.Sprintf("Module[%s]", m.Name())
 }
 
-// Context implements wasm.Module Context
+// Context implements the same method as documented on wasm.Module
 func (m *ModuleContext) Context() context.Context {
 	return m.ctx
 }
@@ -51,7 +56,7 @@ func (m *ModuleContext) Sys() *SysContext {
 	return m.sys
 }
 
-// WithContext implements wasm.Module WithContext
+// WithContext implements the same method as documented on wasm.Module
 func (m *ModuleContext) WithContext(ctx context.Context) publicwasm.Module {
 	if ctx != nil && ctx != m.ctx { // only re-allocate if it will change the effective context
 		return &ModuleContext{module: m.module, memory: m.memory, ctx: ctx, sys: m.sys}
@@ -59,10 +64,10 @@ func (m *ModuleContext) WithContext(ctx context.Context) publicwasm.Module {
 	return m
 }
 
-// Close implements io.Closer
+// Close implements the same method as documented on wasm.Module
 // Note: When there are multiple errors, the error returned is the last one.
 func (m *ModuleContext) Close() (err error) {
-	err = m.store.CloseModule(m.module.Name)
+	err = m.store.CloseModule(m.Name())
 	if sys := m.sys; sys == nil { // ex from ModuleBuilder
 		return
 	} else if err2 := m.sys.Close(); err2 != nil {
diff --git a/wasm/wasm.go b/wasm/wasm.go
index 917b1fd5..fc1ca7fc 100644
--- a/wasm/wasm.go
+++ b/wasm/wasm.go
@@ -63,6 +63,9 @@ func ValueTypeName(t ValueType) string {
 type Module interface {
 	fmt.Stringer
 
+	// Name is the name this module was instantiated with. Exported functions can be imported with this name.
+	Name() string
+
 	// Closer (Close) releases resources allocated for this Module. Using this while having outstanding function calls
 	// is safe. After calling this function, re-instantiating a module for the same name is allowed.
 	io.Closer


Test patch:
diff --git a/internal/wasm/module_context_test.go b/internal/wasm/module_context_test.go
index eda6725c..bcc0d16c 100644
--- a/internal/wasm/module_context_test.go
+++ b/internal/wasm/module_context_test.go
@@ -133,7 +133,7 @@ func TestModuleContext_String(t *testing.T) {
 
 			require.NoError(t, err)
 			require.Equal(t, tc.expected, m.String())
-			require.Equal(t, tc.expected, s.Module(m.module.Name).String())
+			require.Equal(t, tc.expected, s.Module(m.Name()).String())
 		})
 	}
 }
diff --git a/internal/wasm/store_test.go b/internal/wasm/store_test.go
index fd32d38f..11ad0971 100644
--- a/internal/wasm/store_test.go
+++ b/internal/wasm/store_test.go
@@ -184,7 +184,7 @@ func TestStore_hammer(t *testing.T) {
 	imported, err := s.Instantiate(context.Background(), m, importedModuleName, nil)
 	require.NoError(t, err)
 
-	_, ok := s.modules[imported.module.Name]
+	_, ok := s.modules[imported.Name()]
 	require.True(t, ok)
 
 	importingModule := &Module{
@@ -217,7 +217,7 @@ func TestStore_hammer(t *testing.T) {
 	}
 
 	// Close the imported module.
-	require.NoError(t, s.CloseModule(imported.module.Name))
+	require.NoError(t, s.CloseModule(imported.Name()))
 
 	// All instances are freed.
 	require.Len(t, s.modules, 0)
@@ -386,7 +386,7 @@ func TestFunctionInstance_Call(t *testing.T) {
 				TypeSection: []*FunctionType{{}},
 				ImportSection: []*Import{{
 					Type:     ExternTypeFunc,
-					Module:   hm.module.Name,
+					Module:   hm.Name(),
 					Name:     functionName,
 					DescFunc: 0,
 				}},
diff --git a/tests/engine/adhoc_test.go b/tests/engine/adhoc_test.go
index a87c403a..3aaf3cdd 100644
--- a/tests/engine/adhoc_test.go
+++ b/tests/engine/adhoc_test.go
@@ -15,14 +15,13 @@ import (
 )
 
 var tests = map[string]func(t *testing.T, r wazero.Runtime){
-	"huge stack":                                 testHugeStack,
-	"unreachable":                                testUnreachable,
-	"recursive entry":                            testRecursiveEntry,
-	"imported-and-exported func":                 testImportedAndExportedFunc,
-	"host function with context parameter":       testHostFunctionContextParameter,
-	"host function with numeric parameter":       testHostFunctionNumericParameter,
-	"close module with in-flight calls":          testCloseInFlight,
-	"close imported module with in-flight calls": testCloseImportedInFlight,
+	"huge stack":                           testHugeStack,
+	"unreachable":                          testUnreachable,
+	"recursive entry":                      testRecursiveEntry,
+	"imported-and-exported func":           testImportedAndExportedFunc,
+	"host function with context parameter": testHostFunctionContextParameter,
+	"host function with numeric parameter": testHostFunctionNumericParameter,
+	"close module with in-flight calls":    testCloseInFlight,
 }
 
 func TestEngineJIT(t *testing.T) {
@@ -253,58 +252,70 @@ func testHostFunctionNumericParameter(t *testing.T, r wazero.Runtime) {
 	}
 }
 
-func testCloseInFlight(t *testing.T, r wazero.Runtime) {
-	var moduleCloser func() error
-	_, err := r.NewModuleBuilder("host").ExportFunctions(map[string]interface{}{
-		"close_module": func() { _ = moduleCloser() }, // Closing while executing itself.
-	}).Instantiate()
-	require.NoError(t, err)
-
-	m, err := r.InstantiateModuleFromSource([]byte(`(module $test
-	(import "host" "close_module" (func $close_module ))
-
-	(func $close_while_execution
-		call $close_module
+func callImportAfterAddSource(importedModule, importingModule string) []byte {
+	return []byte(fmt.Sprintf(`(module $%[1]s
+	(import "%[2]s" "return_input" (func $block (param i32) (result i32)))
+	(func $call_import_after_add (param i32) (param i32) (result i32)
+		local.get 0
+		local.get 1
+		i32.add
+		call $block
 	)
-	(export "close_while_execution" (func $close_while_execution))
-)`))
-	require.NoError(t, err)
-
-	moduleCloser = m.Close
-
-	_, err = m.ExportedFunction("close_while_execution").Call(nil)
-	require.NoError(t, err)
-
+	(export "call_import_after_add" (func $call_import_after_add))
+)`, importingModule, importedModule))
 }
 
-func testCloseImportedInFlight(t *testing.T, r wazero.Runtime) {
-	importedModule, err := r.NewModuleBuilder("host").ExportFunctions(map[string]interface{}{
-		"already_closed": func() {},
-	}).Instantiate()
-	require.NoError(t, err)
-
-	m, err := r.InstantiateModuleFromSource([]byte(`(module $test
-		(import "host" "already_closed" (func $already_closed ))
-
-		(func $close_parent_before_execution
-			call $already_closed
-		)
-		(export "close_parent_before_execution" (func $close_parent_before_execution))
-	)`))
-	require.NoError(t, err)
-
-	// Closing the imported module before making call should also safe.
-	require.NoError(t, importedModule.Close())
-
-	// Even we can re-enstantiate the module for the same name.
-	importedModuleNew, err := r.NewModuleBuilder("host").ExportFunctions(map[string]interface{}{
-		"already_closed": func() {
-			panic("unreachable") // The new module's function must not be called.
+func testCloseInFlight(t *testing.T, r wazero.Runtime) {
+	tests := []struct {
+		name           string
+		closeImporting bool
+		closeImported  bool
+	}{
+		{
+			name:           "importing", // Ex. WASI proc_exit or AssemblyScript abort handler.
+			closeImporting: true,
 		},
-	}).Instantiate()
-	require.NoError(t, err)
-	defer importedModuleNew.Close() // nolint
+		{
+			name:          "imported",
+			closeImported: true,
+		},
+		{
+			name:           "both", // Ex. A function that stops the runtime.
+			closeImporting: true,
+			closeImported:  true,
+		},
+	}
+	for _, tt := range tests {
+		tc := tt
+
+		t.Run(tc.name, func(t *testing.T) {
+			var imported, importing publicwasm.Module
+			var err error
+			closeAndReturn := func(x uint32) uint32 {
+				if tc.closeImporting {
+					require.NoError(t, importing.Close())
+				}
+				if tc.closeImported {
+					require.NoError(t, importing.Close())
+				}
+				return x
+			}
+
+			// Create the host module, which exports the function that closes the importing module.
+			imported, err = r.NewModuleBuilder(t.Name()+"-imported").
+				ExportFunction("return_input", closeAndReturn).Instantiate()
+			require.NoError(t, err)
+			defer imported.Close()
 
-	_, err = m.ExportedFunction("close_parent_before_execution").Call(nil)
-	require.NoError(t, err)
+			// Import that module.
+			source := callImportAfterAddSource(imported.Name(), t.Name()+"-importing")
+			importing, err = r.InstantiateModuleFromSource(source)
+			require.NoError(t, err)
+			defer importing.Close()
+
+			// Expect no error, because there's currently no logic to fail on the return path after close.
+			_, err = importing.ExportedFunction("call_import_after_add").Call(nil, 1, 2)
+			require.NoError(t, err)
+		})
+	}
 }
diff --git a/tests/engine/hammer_test.go b/tests/engine/hammer_test.go
index f67a1e2e..7a618aa3 100644
--- a/tests/engine/hammer_test.go
+++ b/tests/engine/hammer_test.go
@@ -2,7 +2,6 @@ package adhoc
 
 import (
 	"errors"
-	"runtime"
 	"sync"
 	"sync/atomic"
 	"testing"
@@ -10,6 +9,7 @@ import (
 	"github.com/stretchr/testify/require"
 
 	"github.com/tetratelabs/wazero"
+	"github.com/tetratelabs/wazero/internal/testing/hammer"
 	"github.com/tetratelabs/wazero/wasm"
 )
 
@@ -30,183 +30,97 @@ func TestEngineInterpreter_hammer(t *testing.T) {
 	runAllTests(t, hammers, wazero.NewRuntimeConfigInterpreter())
 }
 
-type blocker struct {
-	running chan bool
-	// unblocked uses a WaitGroup as it allows releasing all goroutines at the same time.
-	unblocked sync.WaitGroup
-	// closed should panic if fn is called when the value is 1.
-	//
-	// Note: Exclusively reading and updating this with atomics guarantees cross-goroutine observations.
-	// See /RATIONALE.md
-	closed uint32
-}
-
-func (b *blocker) close() {
-	atomic.StoreUint32(&b.closed, 1)
+func closeImportingModuleWhileInUse(t *testing.T, r wazero.Runtime) {
+	closeModuleWhileInUse(t, r, func(hostFunctionClosed *uint32, imported, importing wasm.Module) (wasm.Module, wasm.Module) {
+		// Close the importing module, despite calls being in-flight.
+		require.NoError(t, importing.Close())
+
+		// Prove a module can be redefined even with in-flight calls.
+		source := callImportAfterAddSource(imported.Name(), importing.Name())
+		importing, err := r.InstantiateModuleFromSource(source)
+		require.NoError(t, err)
+		return imported, importing
+	})
 }
 
-// fn sends into the running channel, then blocks until it can receive from unblocked one.
-func (b *blocker) fn(input uint32) uint32 {
-	if atomic.LoadUint32(&b.closed) == 1 {
-		panic(errors.New("closed"))
-	}
-	b.running <- true  // Signal the goroutine is running
-	b.unblocked.Wait() // Await until unblocked
-	return input
+func closeImportedModuleWhileInUse(t *testing.T, r wazero.Runtime) {
+	closeModuleWhileInUse(t, r, func(hostFunctionClosed *uint32, imported, importing wasm.Module) (wasm.Module, wasm.Module) {
+		// Close the underlying host function, which causes future calls to it to fail.
+		atomic.StoreUint32(hostFunctionClosed, 1)
+
+		// Validate new calls to the imported function fail, since it was closed.
+		_, err := imported.ExportedFunction("return_input").Call(nil, 1)
+		require.Contains(t, err.Error(), "wasm runtime error: function closed")
+
+		// Close both the importing and the imported module, despite calls being in-flight
+		require.NoError(t, imported.Close())
+		require.NoError(t, importing.Close())
+
+		// Redefine the imported module, with a function that no longer blocks.
+		imported, err = r.NewModuleBuilder(imported.Name()).ExportFunction("return_input", func(x uint32) uint32 {
+			return x
+		}).Instantiate()
+		require.NoError(t, err)
+
+		// Redefine the importing module, which should link to the redefined host module.
+		source := callImportAfterAddSource(imported.Name(), importing.Name())
+		importing, err = r.InstantiateModuleFromSource(source)
+		require.NoError(t, err)
+
+		return imported, importing
+	})
 }
 
-var blockAfterAddSource = []byte(`(module
-	(import "host" "block" (func $block (param i32) (result i32)))
-	(func $block_after_add (param i32) (param i32) (result i32)
-		local.get 0
-		local.get 1
-		i32.add
-		call $block
-	)
-	(export "block_after_add" (func $block_after_add))
-)`)
-
-func closeImportingModuleWhileInUse(t *testing.T, r wazero.Runtime) {
+func closeModuleWhileInUse(t *testing.T, r wazero.Runtime, closeFn func(hostFunctionClosed *uint32, imported, importing wasm.Module) (wasm.Module, wasm.Module)) {
 	args := []uint64{1, 123}
 	exp := args[0] + args[1]
 
-	P := 8               // P+1 == max count of goroutines/in-flight function calls
+	P := 8               // max count of goroutines
 	if testing.Short() { // Adjust down if `-test.short`
 		P = 4
 	}
-	defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(P / 2)) // Ensure goroutines have to switch cores.
-
-	running := make(chan bool)
-	b := &blocker{running: running}
-	b.unblocked.Add(P)
-
-	imported, err := r.NewModuleBuilder("host").ExportFunction("block", b.fn).Instantiate()
-	require.NoError(t, err)
-	defer imported.Close()
-
-	importing, err := r.InstantiateModuleFromSource(blockAfterAddSource)
-	require.NoError(t, err)
-	defer importing.Close()
-
-	// Add channel that tracks P goroutines.
-	done := make(chan int)
-	for p := 0; p < P; p++ {
-		go func() { // Launch goroutine 'p'
-			defer completeGoroutine(t, done)
-
-			// As this is a blocking function, we can only run 1 function per goroutine.
-			requireFunctionCall(t, importing.ExportedFunction("block_after_add"), args, exp)
-		}()
-	}
-
-	// Wait until all functions are in-flight.
-	for i := 0; i < P; i++ {
-		<-running
-	}
-
-	// Close the module that exported blockAfterAdd (noting calls to this are in-flight).
-	require.NoError(t, importing.Close())
-
-	// Prove a module can be redefined even with in-flight calls.
-	importing, err = r.InstantiateModuleFromSource(blockAfterAddSource)
-	require.NoError(t, err)
-	defer importing.Close()
-
-	// If unloading worked properly, a new function call should route to the newly instantiated module.
-	b.unblocked.Add(1)
-	go func() {
-		defer completeGoroutine(t, done)
-
-		requireFunctionCall(t, importing.ExportedFunction("block_after_add"), args, exp)
-	}()
-	<-running // Wait for the above function to be in-flight
-	P++
-
-	// Unblock the functions to ensure they don't err on the return path of a closed module.
-	b.unblocked.Add(-P)
-
-	// Wait for all goroutines to complete
-	for i := 0; i < P; i++ {
-		<-done
-	}
-}
-
-func closeImportedModuleWhileInUse(t *testing.T, r wazero.Runtime) {
-	args := []uint64{1, 123}
-	exp := args[0] + args[1]
 
-	P := 8               // P+1 == max count of goroutines/in-flight function calls
-	if testing.Short() { // Adjust down if `-test.short`
-		P = 4
+	var hostFunctionClosed uint32
+	// To know return path works on a closed module, we need to block calls.
+	var calls sync.WaitGroup
+	calls.Add(P)
+	blockAndReturn := func(x uint32) uint32 {
+		if atomic.LoadUint32(&hostFunctionClosed) == 1 { // Not require.False as we don't want to fail the test.
+			panic(errors.New("function closed"))
+		}
+		calls.Wait()
+		return x
 	}
-	defer runtime.GOMAXPROCS(runtime.GOMAXPROCS(P / 2)) // Ensure goroutines have to switch cores.
-
-	running := make(chan bool)
-	b := &blocker{running: running}
-	b.unblocked.Add(P)
 
-	imported, err := r.NewModuleBuilder("host").ExportFunction("block", b.fn).Instantiate()
+	// Create the host module, which exports the blocking function.
+	imported, err := r.NewModuleBuilder(t.Name()+"-imported").
+		ExportFunction("return_input", blockAndReturn).Instantiate()
 	require.NoError(t, err)
 	defer imported.Close()
 
-	importing, err := r.InstantiateModuleFromSource(blockAfterAddSource)
+	// Import that module.
+	source := callImportAfterAddSource(imported.Name(), t.Name()+"-importing")
+	importing, err := r.InstantiateModuleFromSource(source)
 	require.NoError(t, err)
 	defer importing.Close()
 
-	// Add channel that tracks P goroutines.
-	done := make(chan int)
-	for p := 0; p < P; p++ {
-		go func() { // Launch goroutine 'p'
-			defer completeGoroutine(t, done)
-
-			// As this is a blocking function, we can only run 1 function per goroutine.
-			requireFunctionCall(t, importing.ExportedFunction("block_after_add"), args, exp)
-		}()
-	}
-
-	// Wait until all functions are in-flight.
-	for i := 0; i < P; i++ {
-		<-running
-	}
-
-	// Close the module that exported the host function (noting calls to this are in-flight).
-	require.NoError(t, imported.Close())
-	// Close the underlying host function, which causes future calls to it to fail.
-	b.close()
-	require.Panics(t, func() {
-		b.fn(1) // validate it would fail if accidentally called
+	// As this is a blocking function call, only run 1 per goroutine.
+	hammer.NewHammer(t, P, 1).Run(func(name string) {
+		requireFunctionCall(t, importing.ExportedFunction("call_import_after_add"), args, exp)
+	}, func() { // When all functions are in-flight, re-assign the modules.
+		imported, importing = closeFn(&hostFunctionClosed, imported, importing)
+		// Unblock all the calls
+		calls.Add(-P)
 	})
-	// Close the importing module
-	require.NoError(t, importing.Close())
-
-	// Prove a host module can be redefined even with in-flight calls.
-	b1 := &blocker{running: running} // New instance, so not yet closed!
-	imported, err = r.NewModuleBuilder("host").ExportFunction("block", b1.fn).Instantiate()
-	require.NoError(t, err)
+	// As references may have changed, ensure we close both.
 	defer imported.Close()
-
-	// Redefine the importing module, which should link to the redefined host module.
-	importing, err = r.InstantiateModuleFromSource(blockAfterAddSource)
-	require.NoError(t, err)
 	defer importing.Close()
+	if t.Failed() {
+		return // At least one test failed, so return now.
+	}
 
 	// If unloading worked properly, a new function call should route to the newly instantiated module.
-	b1.unblocked.Add(1)
-	go func() {
-		defer completeGoroutine(t, done)
-
-		requireFunctionCall(t, importing.ExportedFunction("block_after_add"), args, exp)
-	}()
-	<-running // Wait for the above function to be in-flight
-
-	// Unblock the functions to ensure they don't err on the return path of a closed module.
-	b.unblocked.Add(-P)
-	b1.unblocked.Add(-1)
-
-	// Wait for all goroutines to complete
-	for i := 0; i < P+1; i++ {
-		<-done
-	}
+	requireFunctionCall(t, importing.ExportedFunction("call_import_after_add"), args, exp)
 }
 
 func requireFunctionCall(t *testing.T, fn wasm.Function, args []uint64, exp uint64) {
@@ -215,11 +129,3 @@ func requireFunctionCall(t *testing.T, fn wasm.Function, args []uint64, exp uint
 	require.NoError(t, err)
 	require.Equal(t, exp, res[0])
 }
-
-func completeGoroutine(t *testing.T, c chan int) {
-	// Ensure each require.XX failure is visible on hammer test fail.
-	if recovered := recover(); recovered != nil {
-		t.Error(recovered.(string))
-	}
-	c <- 1
-}