Fixes race on engine creation with Cache API (#1021)

Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>

.github/workflows/commit.yaml

@@ -76,6 +76,11 @@ jobs:
           cache: true
 
       - run: make test
+        if: ${{ github.event_name  == 'pull_request' }}
+
+      # Run tests with -race only on main branch push.
+      - run: make test go_test_options='-race'
+        if: ${{ github.event_name  == 'push' }}
 
       - name: "Generate coverage report"  # only once (not per OS)
         if: runner.os == 'Linux'

Makefile

@@ -22,6 +22,9 @@ main_sources  := $(wildcard $(filter-out %_test.go $(all_testdata) $(all_testing
 # Paths need to all start with ./, so we do that manually vs foreach which strips it.
 main_packages := $(sort $(foreach f,$(dir $(main_sources)),$(if $(findstring ./,$(f)),./,./$(f))))
 
+# By default, we don't run with -race as it's costly to run on every PR.
+go_test_options ?=
+
 ensureCompilerFastest := -ldflags '-X github.com/tetratelabs/wazero/internal/integration_test/vs.ensureCompilerFastest=true'
 .PHONY: bench
 bench:
@@ -45,7 +48,7 @@ build.bench:
 
 .PHONY: test.examples
 test.examples:
-	@go test ./examples/... ./imports/assemblyscript/example/... ./imports/emscripten/... ./imports/go/example/... ./imports/wasi_snapshot_preview1/example/...
+	@go test $(go_test_options) ./examples/... ./imports/assemblyscript/example/... ./imports/emscripten/... ./imports/go/example/... ./imports/wasi_snapshot_preview1/example/...
 
 .PHONY: build.examples.as
 build.examples.as:
@@ -174,8 +177,8 @@ build.spectest.v2: # Note: SIMD cases are placed in the "simd" subdirectory.
 
 .PHONY: test
 test:
-	@go test $$(go list ./... | grep -vE '$(spectest_v1_dir)|$(spectest_v2_dir)') -timeout 120s
+	@go test $(go_test_options) $$(go list ./... | grep -vE '$(spectest_v1_dir)|$(spectest_v2_dir)') -timeout 120s
-	@cd internal/version/testdata && go test ./... -timeout 120s
+	@cd internal/version/testdata && go test $(go_test_options) ./... -timeout 120s
 
 .PHONY: coverage
 coverpkg = $(subst $(space),$(comma),$(main_packages))
@@ -189,10 +192,10 @@ spectest:
 	@$(MAKE) spectest.v2
 
 spectest.v1:
-	@go test $$(go list ./... | grep $(spectest_v1_dir)) -timeout 120s
+	@go test $(go_test_options) $$(go list ./... | grep $(spectest_v1_dir)) -timeout 120s
 
 spectest.v2:
-	@go test $$(go list ./... | grep $(spectest_v2_dir)) -timeout 120s
+	@go test $(go_test_options) $$(go list ./... | grep $(spectest_v2_dir)) -timeout 120s
 
 golangci_lint_path := $(shell go env GOPATH)/bin/golangci-lint
 

cache.go

@@ -8,6 +8,7 @@ import (
 	"path"
 	"path/filepath"
 	goruntime "runtime"
+	"sync"
 
 	"github.com/tetratelabs/wazero/api"
 	"github.com/tetratelabs/wazero/internal/filecache"
@@ -50,9 +51,14 @@ func NewCompilationCacheWithDir(dirname string) (CompilationCache, error) {
 type cache struct {
 	// eng is the engine for this cache. If the cache is configured, the engine is shared across multiple instances of
 	// Runtime, and its lifetime is not bound to them. Instead, the engine is alive until Cache.Close is called.
-	eng wasm.Engine
+	eng       wasm.Engine
-
 	fileCache filecache.Cache
+	once      sync.Once
+}
+
+func (c *cache) initEngine(ne newEngine, ctx context.Context, features api.CoreFeatures) wasm.Engine {
+	c.once.Do(func() { c.eng = ne(ctx, features, c.fileCache) })
+	return c.eng
 }
 
 // Close implements the same method on the Cache interface.

config.go

@@ -125,13 +125,15 @@ func NewRuntimeConfig() RuntimeConfig {
 	return newRuntimeConfig()
 }
 
+type newEngine func(context.Context, api.CoreFeatures, filecache.Cache) wasm.Engine
+
 type runtimeConfig struct {
 	enabledFeatures       api.CoreFeatures
 	memoryLimitPages      uint32
 	memoryCapacityFromMax bool
 	isInterpreter         bool
 	dwarfDisabled         bool // negative as defaults to enabled
-	newEngine             func(context.Context, api.CoreFeatures, filecache.Cache) wasm.Engine
+	newEngine             newEngine
 	cache                 CompilationCache
 }
 

runtime.go

@@ -116,10 +116,7 @@ func NewRuntimeWithConfig(ctx context.Context, rConfig RuntimeConfig) Runtime {
 	if c := config.cache; c != nil {
 		// If the Cache is configured, we share the engine.
 		cacheImpl = c.(*cache)
-		if cacheImpl.eng == nil {
+		engine = cacheImpl.initEngine(config.newEngine, ctx, config.enabledFeatures)
-			cacheImpl.eng = config.newEngine(ctx, config.enabledFeatures, cacheImpl.fileCache)
-		}
-		engine = cacheImpl.eng
 	} else {
 		// Otherwise, we create a new engine.
 		engine = config.newEngine(ctx, config.enabledFeatures, nil)

runtime_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	_ "embed"
 	"errors"
+	"sync"
 	"testing"
 	"time"
 
@@ -695,3 +696,21 @@ func (e *mockEngine) Close() (err error) {
 	e.closed = true
 	return
 }
+
+// TestNewRuntime_concurrent ensures that concurrent execution of NewRuntime is race-free.
+// This depends on -race flag.
+func TestNewRuntime_concurrent(t *testing.T) {
+	const num = 100
+	var wg sync.WaitGroup
+	config := NewRuntimeConfig().WithCompilationCache(NewCompilationCache())
+	wg.Add(num)
+	for i := 0; i < num; i++ {
+		go func() {
+			defer wg.Done()
+			r := NewRuntimeWithConfig(testCtx, config)
+			err := r.Close(testCtx)
+			require.NoError(t, err)
+		}()
+	}
+	wg.Wait()
+}