Repository: tetratelabs/wazero (go)

Issue:
Consider not returning api.Module when instantiating wasi
I noticed that the module returned from wasi instantiation isn't usable as a normal module - notably it doesn't have `Memory` or `Sys`, so trying to call exported functions doesn't work. wasi module is meant to be a supporting module imported by another wasm module. `Module` is likely returned to allow `Close` - now that we have `Runtime.Close` we could consider not returning a module when initializing wasi to prevent users (like me :-) ) from trying to use it as a normal module. It does mean that `Runtime.Close` stops being an alternative to closing modules but is required when using wasi - this may be natural because people often call `Close` anytime there is a `Close` method I guess.

It could also be ok to leave behavior as is if there is a reasonable runtime message. Unfortunately, wasi functions often requiring the host to write into `Memory` before calling. This means that the error can easily be a nil reference exception in the user code when trying to use `Memory`, without a good error message.

Pull request description:
Adds a Closer interface to encapsulate the API for closing a re…
…source with modules.

Fixes #551 

`api.Module` provides APIs for using a wasm module directly such as `Memory()` and `ExportedFunction`. Built-in modules such as wasi do not expose such functionality, but do still need to be closeable. `Closer` provides the functionality for closing a module and can be returned in these cases. It is also a natural fit with `Runtime`, which has the ability to close modules despite not being a module itself.

Patch:
diff --git a/api/wasm.go b/api/wasm.go
index f366ef70..8c101e7d 100644
--- a/api/wasm.go
+++ b/api/wasm.go
@@ -129,22 +129,6 @@ type Module interface {
 	// Name is the name this module was instantiated with. Exported functions can be imported with this name.
 	Name() string
 
-	// Close is a convenience that invokes CloseWithExitCode with zero.
-	// Note: When the context is nil, it defaults to context.Background.
-	Close(context.Context) error
-
-	// CloseWithExitCode releases resources allocated for this Module. Use a non-zero exitCode parameter to indicate a
-	// failure to ExportedFunction callers.
-	//
-	// The error returned here, if present, is about resource de-allocation (such as I/O errors). Only the last error is
-	// returned, so a non-nil return means at least one error happened. Regardless of error, this module instance will
-	// be removed, making its name available again.
-	//
-	// Calling this inside a host function is safe, and may cause ExportedFunction callers to receive a sys.ExitError
-	// with the exitCode.
-	// Note: When the context is nil, it defaults to context.Background.
-	CloseWithExitCode(ctx context.Context, exitCode uint32) error
-
 	// Memory returns a memory defined in this module or nil if there are none wasn't.
 	Memory() Memory
 
@@ -162,6 +146,30 @@ type Module interface {
 
 	// ExportedGlobal a global exported from this module or nil if it wasn't.
 	ExportedGlobal(name string) Global
+
+	// CloseWithExitCode releases resources allocated for this Module. Use a non-zero exitCode parameter to indicate a
+	// failure to ExportedFunction callers.
+	//
+	// The error returned here, if present, is about resource de-allocation (such as I/O errors). Only the last error is
+	// returned, so a non-nil return means at least one error happened. Regardless of error, this module instance will
+	// be removed, making its name available again.
+	//
+	// Calling this inside a host function is safe, and may cause ExportedFunction callers to receive a sys.ExitError
+	// with the exitCode.
+	// Note: When the context is nil, it defaults to context.Background.
+	CloseWithExitCode(ctx context.Context, exitCode uint32) error
+
+	// Closer closes this module by delegating to CloseWithExitCode with an exit code of zero.
+	Closer
+}
+
+// Closer closes a resource.
+//
+// Note: This is an interface for decoupling, not third-party implementations. All implementations are in wazero.
+type Closer interface {
+	// Close closes the resource.
+	// Note: When the context is nil, it defaults to context.Background.
+	Close(context.Context) error
 }
 
 // Function is a WebAssembly 1.0 (20191205) function exported from an instantiated module (wazero.Runtime InstantiateModule).
diff --git a/wasi/wasi.go b/wasi/wasi.go
index 8288847c..6e2e0949 100644
--- a/wasi/wasi.go
+++ b/wasi/wasi.go
@@ -36,8 +36,8 @@ const ModuleSnapshotPreview1 = "wasi_snapshot_preview1"
 //	mod, _ := r.InstantiateModuleFromCode(ctx, source)
 //
 // Note: All WASI functions return a single Errno result, ErrnoSuccess on success.
-// Note: Closing the wazero.Runtime closes any api.Module it instantiated.
-func InstantiateSnapshotPreview1(ctx context.Context, r wazero.Runtime) (api.Module, error) {
+// Note: Closing the wazero.Runtime closes this instance of WASI as well.
+func InstantiateSnapshotPreview1(ctx context.Context, r wazero.Runtime) (api.Closer, error) {
 	_, fns := snapshotPreview1Functions(ctx)
 	return r.NewModuleBuilder(ModuleSnapshotPreview1).ExportFunctions(fns).Instantiate(ctx)
 }
diff --git a/wasm.go b/wasm.go
index ce73069c..bd34be14 100644
--- a/wasm.go
+++ b/wasm.go
@@ -98,19 +98,6 @@ type Runtime interface {
 	// Note: When the context is nil, it defaults to context.Background.
 	InstantiateModule(ctx context.Context, compiled CompiledModule, config ModuleConfig) (api.Module, error)
 
-	// Close closes all the modules that have been initialized in this Runtime with an exit code of 0.
-	// An error is returned if any module returns an error when closed.
-	//
-	// Ex.
-	//	ctx := context.Background()
-	//	r := wazero.NewRuntime()
-	//	defer r.Close(ctx) // This closes everything this Runtime created.
-	//
-	//	// Everything below here can be closed, but will anyway due to above.
-	//	_, _ = wasi.InstantiateSnapshotPreview1(ctx, r)
-	//	mod, _ := r.InstantiateModuleFromCode(ctx, source)
-	Close(context.Context) error
-
 	// CloseWithExitCode closes all the modules that have been initialized in this Runtime with the provided exit code.
 	// An error is returned if any module returns an error when closed.
 	//
@@ -123,6 +110,10 @@ type Runtime interface {
 	//	_, _ = wasi.InstantiateSnapshotPreview1(ctx, r)
 	//	mod, _ := r.InstantiateModuleFromCode(ctx, source)
 	CloseWithExitCode(ctx context.Context, exitCode uint32) error
+
+	// Closer closes resources initialized by this Runtime by delegating to CloseWithExitCode with an exit code of
+	// zero.
+	api.Closer
 }
 
 func NewRuntime() Runtime {


Test patch:
diff --git a/internal/integration_test/fs/fs_test.go b/internal/integration_test/fs/fs_test.go
new file mode 100644
index 00000000..bb414c64
--- /dev/null
+++ b/internal/integration_test/fs/fs_test.go
@@ -0,0 +1,198 @@
+package fs
+
+import (
+	"context"
+	_ "embed"
+	"fmt"
+	"io"
+	"io/fs"
+	"testing"
+	"testing/fstest"
+	"testing/iotest"
+
+	"github.com/tetratelabs/wazero"
+	"github.com/tetratelabs/wazero/api"
+	"github.com/tetratelabs/wazero/internal/testing/require"
+	"github.com/tetratelabs/wazero/wasi"
+)
+
+var testCtx = context.Background()
+
+//go:embed testdata/animals.txt
+var animals []byte
+
+// wasiFs is an implementation of fs.Fs calling into wasi. Not thread-safe because we use
+// fixed Memory offsets for transferring data with wasm.
+type wasiFs struct {
+	t *testing.T
+
+	wasm   wazero.Runtime
+	memory api.Memory
+
+	workdirFd uint32
+
+	pathOpen api.Function
+	fdClose  api.Function
+	fdRead   api.Function
+	fdSeek   api.Function
+}
+
+func (fs *wasiFs) Open(name string) (fs.File, error) {
+	pathBytes := []byte(name)
+	// Pick anywhere in memory to write the path to.
+	pathPtr := uint32(0)
+	ok := fs.memory.Write(testCtx, pathPtr, pathBytes)
+	require.True(fs.t, ok)
+	resultOpenedFd := pathPtr + uint32(len(pathBytes))
+
+	fd := fs.workdirFd
+	dirflags := uint32(0) // arbitrary dirflags
+	pathLen := len(pathBytes)
+	oflags := uint32(0) // arbitrary oflags
+	// rights are ignored per https://github.com/WebAssembly/WASI/issues/469#issuecomment-1045251844
+	fsRightsBase, fsRightsInheriting := uint64(1), uint64(2)
+	fdflags := uint32(0) // arbitrary fdflags
+	res, err := fs.pathOpen.Call(
+		testCtx,
+		uint64(fd), uint64(dirflags), uint64(pathPtr), uint64(pathLen), uint64(oflags),
+		fsRightsBase, fsRightsInheriting, uint64(fdflags), uint64(resultOpenedFd))
+	require.NoError(fs.t, err)
+	require.Equal(fs.t, uint64(wasi.ErrnoSuccess), res[0])
+
+	resFd, ok := fs.memory.ReadUint32Le(testCtx, resultOpenedFd)
+	require.True(fs.t, ok)
+
+	return &wasiFile{fd: resFd, fs: fs}, nil
+}
+
+// wasiFile implements io.Reader and io.Seeker using wasi functions. It does not
+// implement io.ReaderAt because there is no wasi function for directly reading
+// from an offset.
+type wasiFile struct {
+	fd uint32
+	fs *wasiFs
+}
+
+func (f *wasiFile) Stat() (fs.FileInfo, error) {
+	// We currently don't implement wasi's fd_stat but also don't use this method from this test.
+	panic("unused")
+}
+
+func (f *wasiFile) Read(bytes []byte) (int, error) {
+	// Pick anywhere in memory for wasm to write resultSize too. We do this first since it's fixed length
+	// while iovs is variable.
+	resultSizeOff := uint32(0)
+	// Next place iovs
+	iovsOff := uint32(4)
+	// We do not directly write to hardware, there is no need for more than one iovec
+	iovsCount := uint32(1)
+	// iov starts at iovsOff + 8 because we first write four bytes for the offset itself, and
+	// four bytes for the length of the iov.
+	iovOff := iovsOff + uint32(8)
+	ok := f.fs.memory.WriteUint32Le(testCtx, iovsOff, iovOff)
+	require.True(f.fs.t, ok)
+	// Next write the length.
+	ok = f.fs.memory.WriteUint32Le(testCtx, iovsOff+uint32(4), uint32(len(bytes)))
+	require.True(f.fs.t, ok)
+
+	res, err := f.fs.fdRead.Call(testCtx, uint64(f.fd), uint64(iovsOff), uint64(iovsCount), uint64(resultSizeOff))
+	require.NoError(f.fs.t, err)
+
+	require.NotEqual(f.fs.t, uint64(wasi.ErrnoFault), res[0])
+
+	numRead, ok := f.fs.memory.ReadUint32Le(testCtx, resultSizeOff)
+	require.True(f.fs.t, ok)
+
+	if numRead == 0 {
+		if len(bytes) == 0 {
+			return 0, nil
+		}
+		if wasi.Errno(res[0]) == wasi.ErrnoSuccess {
+			return 0, io.EOF
+		} else {
+			return 0, fmt.Errorf("could not read from file")
+		}
+	}
+
+	buf, ok := f.fs.memory.Read(testCtx, iovOff, numRead)
+	require.True(f.fs.t, ok)
+	copy(bytes, buf)
+	return int(numRead), nil
+}
+
+func (f *wasiFile) Close() error {
+	res, err := f.fs.fdClose.Call(testCtx, uint64(f.fd))
+	require.NoError(f.fs.t, err)
+	require.NotEqual(f.fs.t, uint64(wasi.ErrnoFault), res[0])
+	return nil
+}
+
+func (f *wasiFile) Seek(offset int64, whence int) (int64, error) {
+	// Pick anywhere in memory for wasm to write the result newOffset to
+	resultNewoffsetOff := uint32(0)
+
+	res, err := f.fs.fdSeek.Call(testCtx, uint64(f.fd), uint64(offset), uint64(whence), uint64(resultNewoffsetOff))
+	require.NoError(f.fs.t, err)
+	require.NotEqual(f.fs.t, uint64(wasi.ErrnoFault), res[0])
+
+	newOffset, ok := f.fs.memory.ReadUint32Le(testCtx, resultNewoffsetOff)
+	require.True(f.fs.t, ok)
+
+	return int64(newOffset), nil
+}
+
+func TestReader(t *testing.T) {
+	r := wazero.NewRuntime()
+	defer r.Close(testCtx)
+
+	_, err := wasi.InstantiateSnapshotPreview1(testCtx, r)
+	require.NoError(t, err)
+
+	realFs := fstest.MapFS{"animals.txt": &fstest.MapFile{Data: animals}}
+	sys := wazero.NewModuleConfig().WithWorkDirFS(realFs)
+
+	// Create a module that just delegates to wasi functions.
+	compiled, err := r.CompileModule(testCtx, []byte(`(module
+  (import "wasi_snapshot_preview1" "path_open"
+    (func $wasi.path_open (param $fd i32) (param $dirflags i32) (param $path i32) (param $path_len i32) (param $oflags i32) (param $fs_rights_base i64) (param $fs_rights_inheriting i64) (param $fdflags i32) (param $result.opened_fd i32) (result (;errno;) i32)))
+  (import "wasi_snapshot_preview1" "fd_close"
+    (func $wasi.fd_close (param $fd i32) (result (;errno;) i32)))
+  (import "wasi_snapshot_preview1" "fd_read"
+    (func $wasi.fd_read (param $fd i32) (param $iovs i32) (param $iovs_len i32) (param $result.size i32) (result (;errno;) i32)))
+  (import "wasi_snapshot_preview1" "fd_seek"
+    (func $wasi.fd_seek (param $fd i32) (param $offset i64) (param $whence i32) (param $result.newoffset i32) (result (;errno;) i32)))
+  (memory 1 1)  ;; just an arbitrary size big enough for tests
+  (export "memory" (memory 0))
+  (export "path_open" (func $wasi.path_open))
+  (export "fd_close" (func $wasi.fd_close))
+  (export "fd_read" (func $wasi.fd_read))
+  (export "fd_seek" (func $wasi.fd_seek))
+)`), wazero.NewCompileConfig())
+	require.NoError(t, err)
+
+	mod, err := r.InstantiateModule(testCtx, compiled, sys)
+	require.NoError(t, err)
+
+	pathOpen := mod.ExportedFunction("path_open")
+	fdClose := mod.ExportedFunction("fd_close")
+	fdRead := mod.ExportedFunction("fd_read")
+	fdSeek := mod.ExportedFunction("fd_seek")
+
+	wasiFs := &wasiFs{
+		t:         t,
+		wasm:      r,
+		memory:    mod.Memory(),
+		workdirFd: uint32(3),
+		pathOpen:  pathOpen,
+		fdClose:   fdClose,
+		fdRead:    fdRead,
+		fdSeek:    fdSeek,
+	}
+
+	f, err := wasiFs.Open("animals.txt")
+	require.NoError(t, err)
+	defer f.Close()
+
+	err = iotest.TestReader(f, animals)
+	require.NoError(t, err)
+}
diff --git a/internal/integration_test/fs/testdata/animals.txt b/internal/integration_test/fs/testdata/animals.txt
new file mode 100644
index 00000000..b5b2f23d
--- /dev/null
+++ b/internal/integration_test/fs/testdata/animals.txt
@@ -0,0 +1,5 @@
+bear
+cat
+shark
+dinosaur
+human
diff --git a/internal/integration_test/vs/runtime.go b/internal/integration_test/vs/runtime.go
index 28d104ec..7cff9c02 100644
--- a/internal/integration_test/vs/runtime.go
+++ b/internal/integration_test/vs/runtime.go
@@ -58,8 +58,9 @@ type wazeroRuntime struct {
 }
 
 type wazeroModule struct {
-	wasi, env, mod api.Module
-	funcs          map[string]api.Function
+	wasi     api.Closer
+	env, mod api.Module
+	funcs    map[string]api.Function
 }
 
 func (r *wazeroRuntime) Name() string {