From c5a162e2b0be71bcc00e9029d18dcc7d2a28a935 Mon Sep 17 00:00:00 2001 From: oleiade Date: Wed, 5 Jul 2023 11:11:16 +0200 Subject: [PATCH 01/18] Implement `fs` module with open method, File and FileInfo abstractions --- js/modules/k6/experimental/fs/cache.go | 86 +++++ js/modules/k6/experimental/fs/cache_test.go | 70 ++++ js/modules/k6/experimental/fs/errors.go | 68 ++++ js/modules/k6/experimental/fs/errors_gen.go | 87 +++++ js/modules/k6/experimental/fs/file.go | 28 ++ js/modules/k6/experimental/fs/module.go | 175 +++++++++ js/modules/k6/experimental/fs/module_test.go | 359 +++++++++++++++++++ 7 files changed, 873 insertions(+) create mode 100644 js/modules/k6/experimental/fs/cache.go create mode 100644 js/modules/k6/experimental/fs/cache_test.go create mode 100644 js/modules/k6/experimental/fs/errors.go create mode 100644 js/modules/k6/experimental/fs/errors_gen.go create mode 100644 js/modules/k6/experimental/fs/file.go create mode 100644 js/modules/k6/experimental/fs/module.go create mode 100644 js/modules/k6/experimental/fs/module_test.go diff --git a/js/modules/k6/experimental/fs/cache.go b/js/modules/k6/experimental/fs/cache.go new file mode 100644 index 00000000000..2602d2821d2 --- /dev/null +++ b/js/modules/k6/experimental/fs/cache.go @@ -0,0 +1,86 @@ +package fs + +import ( + "fmt" + "io" + "path/filepath" + "sync" + + "github.com/spf13/afero" +) + +// cache is a cache of opened files. +type cache struct { + // openedFiles holds a safe for concurrent use map, holding the content + // of the files that were opened by the user. + // + // Keys are expected to be strings holding the openedFiles' path. + // Values are expected to be byte slices holding the content of the opened file. + // + // That way, we can cache the file's content and avoid opening too many + // file descriptor, and re-reading its content every time the file is opened. + // + // Importantly, this also means that if the + // file is modified from outside of k6, the changes will not be reflected in the file's data. + openedFiles sync.Map +} + +// open retrieves the content of a given file from the specified filesystem (fromFs) and +// stores it in the registry's internal `openedFiles` map. +// +// The function cleans the provided filename using filepath.Clean before using it. +// +// If the file was previously "opened" (and thus cached) by the registry, it +// returns the cached content. Otherwise, it reads the file from the +// filesystem, caches its content, and then returns it. +// +// The function is designed to minimize redundant file reads by leveraging an internal cache (openedFiles). +// In case the cached value is not a byte slice (which should never occur in regular use), it +// panics with a descriptive error. +// +// Parameters: +// - filename: The name of the file to be retrieved. This should be a relative or absolute path. +// - fromFs: The filesystem (from the afero package) from which the file should be read if not already cached. +// +// Returns: +// - A byte slice containing the content of the specified file. +// - An error if there's any issue opening or reading the file. If the file content is +// successfully cached and returned once, subsequent calls will not produce +// file-related errors for the same file, as the cached value will be used. +func (fr *cache) open(filename string, fromFs afero.Fs) (data []byte, err error) { + filename = filepath.Clean(filename) + + if f, ok := fr.openedFiles.Load(filename); ok { + data, ok = f.([]byte) + if !ok { + panic(fmt.Errorf("registry's file %s is not stored as a byte slice", filename)) + } + + return data, nil + } + + // The underlying afero.Fs.Open method will cache the file content during this + // operation. Which will lead to effectively holding the content of the file in memory twice. + // However, as per #1079, we plan to eventually reduce our dependency on afero, and + // expect this issue to be resolved at that point. + // TODO: re-evaluate opening from the FS this once #1079 is resolved. + f, err := fromFs.Open(filename) + if err != nil { + return nil, err + } + defer func() { + cerr := f.Close() + if cerr != nil { + err = fmt.Errorf("failed to close file %s: %w", filename, cerr) + } + }() + + data, err = io.ReadAll(f) + if err != nil { + return nil, err + } + + fr.openedFiles.Store(filename, data) + + return data, nil +} diff --git a/js/modules/k6/experimental/fs/cache_test.go b/js/modules/k6/experimental/fs/cache_test.go new file mode 100644 index 00000000000..b8dd7218ec6 --- /dev/null +++ b/js/modules/k6/experimental/fs/cache_test.go @@ -0,0 +1,70 @@ +package fs + +import ( + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" +) + +func TestFileCacheOpen(t *testing.T) { + t.Parallel() + + t.Run("open succeeds", func(t *testing.T) { + t.Parallel() + + cache := &cache{} + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, "bonjour.txt", []byte("Bonjour, le monde"), 0o644) + }) + + _, gotBeforeOk := cache.openedFiles.Load("bonjour.txt") + gotData, gotErr := cache.open("bonjour.txt", fs) + _, gotAfterOk := cache.openedFiles.Load("bonjour.txt") + + assert.False(t, gotBeforeOk) + assert.NoError(t, gotErr) + assert.Equal(t, []byte("Bonjour, le monde"), gotData) + assert.True(t, gotAfterOk) + }) + + t.Run("double open succeeds", func(t *testing.T) { + t.Parallel() + + cache := &cache{} + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, "bonjour.txt", []byte("Bonjour, le monde"), 0o644) + }) + + firstData, firstErr := cache.open("bonjour.txt", fs) + _, gotFirstOk := cache.openedFiles.Load("bonjour.txt") + secondData, secondErr := cache.open("bonjour.txt", fs) + _, gotSecondOk := cache.openedFiles.Load("bonjour.txt") + + assert.True(t, gotFirstOk) + assert.NoError(t, firstErr) + assert.Equal(t, []byte("Bonjour, le monde"), firstData) + assert.True(t, gotSecondOk) + assert.NoError(t, secondErr) + assert.True(t, sameUnderlyingArray(firstData, secondData)) + assert.Equal(t, []byte("Bonjour, le monde"), secondData) + }) +} + +// sameUnderlyingArray returns true if the underlying array of lhs and rhs are the same. +// +// This is done by checking that the two slices have a capacity greater than 0 and that +// the last element of the underlying array is the same for both slices. +// +// Once a slice is created, its starting address can move forward, but can never move +// behond its starting address + its capacity, which is a fixed value for any Go slice. +// +// Hence, if the last element of the underlying array is the same for both slices, it +// means that the underlying array is the same. +// +// See [explanation] for more details. +// +// [explanation]: https://groups.google.com/g/golang-nuts/c/ks1jvoyMYuc?pli=1 +func sameUnderlyingArray(lhs, rhs []byte) bool { + return cap(lhs) > 0 && cap(rhs) > 0 && &lhs[0:cap(lhs)][cap(lhs)-1] == &rhs[0:cap(rhs)][cap(rhs)-1] +} diff --git a/js/modules/k6/experimental/fs/errors.go b/js/modules/k6/experimental/fs/errors.go new file mode 100644 index 00000000000..c31a52c72c1 --- /dev/null +++ b/js/modules/k6/experimental/fs/errors.go @@ -0,0 +1,68 @@ +package fs + +// newFsError creates a new Error object of the provided kind and with the +// provided message. +func newFsError(k errorKind, message string) *fsError { + return &fsError{ + Name: k.String(), + Message: message, + kind: k, + } +} + +// errorKind indicates the kind of file system error that has occurred. +// +// Its string representation is generated by the `enumer` tool. The +// `enumer` tool is run by the `go generate` command. See the `go generate` +// command documentation. +// The tool itself is not tracked as part of the k6 go.mod file, and +// therefore must be installed manually using `go install github.com/dmarkham/enumer`. +// +//go:generate enumer -type=errorKind -output errors_gen.go +type errorKind uint8 + +const ( + // NotFoundError is emitted when a file is not found. + NotFoundError errorKind = iota + 1 + + // InvalidResourceError is emitted when a resource is invalid: for + // instance when attempting to open a directory, which is not supported. + InvalidResourceError + + // ForbiddenError is emitted when an operation is forbidden. + ForbiddenError + + // TypeError is emitted when an incorrect type has been used. + TypeError +) + +// fsError represents a custom error object emitted by the fs module. +// +// It is used to provide a more detailed error message to the user, and +// provide a concrete error type that can be used to differentiate between +// different types of errors. +// +// Exposing error types to the user in a way that's compatible with some +// JavaScript error handling constructs such as `instanceof` is still non-trivial +// in Go. See the [dedicated goja issue] with have opened for more details. +// +// [dedicated goja issue]: https://github.com/dop251/goja/issues/529 +type fsError struct { + // Name contains the name of the error as formalized by the [ErrorKind] + // type. + Name string `json:"name"` + + // Message contains the error message as presented to the user. + Message string `json:"message"` + + // kind contains the kind of error that has occurred. + kind errorKind +} + +// Ensure that the Error type implements the Go `error` interface. +var _ error = (*fsError)(nil) + +// Error implements the Go `error` interface. +func (e *fsError) Error() string { + return e.Name + ": " + e.Message +} diff --git a/js/modules/k6/experimental/fs/errors_gen.go b/js/modules/k6/experimental/fs/errors_gen.go new file mode 100644 index 00000000000..7ffc4a49d3c --- /dev/null +++ b/js/modules/k6/experimental/fs/errors_gen.go @@ -0,0 +1,87 @@ +// Code generated by "enumer -type=errorKind -output errors_gen.go"; DO NOT EDIT. + +package fs + +import ( + "fmt" + "strings" +) + +const _errorKindName = "NotFoundErrorInvalidResourceErrorForbiddenErrorTypeError" + +var _errorKindIndex = [...]uint8{0, 13, 33, 47, 56} + +const _errorKindLowerName = "notfounderrorinvalidresourceerrorforbiddenerrortypeerror" + +func (i errorKind) String() string { + i -= 1 + if i >= errorKind(len(_errorKindIndex)-1) { + return fmt.Sprintf("errorKind(%d)", i+1) + } + return _errorKindName[_errorKindIndex[i]:_errorKindIndex[i+1]] +} + +// An "invalid array index" compiler error signifies that the constant values have changed. +// Re-run the stringer command to generate them again. +func _errorKindNoOp() { + var x [1]struct{} + _ = x[NotFoundError-(1)] + _ = x[InvalidResourceError-(2)] + _ = x[ForbiddenError-(3)] + _ = x[TypeError-(4)] +} + +var _errorKindValues = []errorKind{NotFoundError, InvalidResourceError, ForbiddenError, TypeError} + +var _errorKindNameToValueMap = map[string]errorKind{ + _errorKindName[0:13]: NotFoundError, + _errorKindLowerName[0:13]: NotFoundError, + _errorKindName[13:33]: InvalidResourceError, + _errorKindLowerName[13:33]: InvalidResourceError, + _errorKindName[33:47]: ForbiddenError, + _errorKindLowerName[33:47]: ForbiddenError, + _errorKindName[47:56]: TypeError, + _errorKindLowerName[47:56]: TypeError, +} + +var _errorKindNames = []string{ + _errorKindName[0:13], + _errorKindName[13:33], + _errorKindName[33:47], + _errorKindName[47:56], +} + +// errorKindString retrieves an enum value from the enum constants string name. +// Throws an error if the param is not part of the enum. +func errorKindString(s string) (errorKind, error) { + if val, ok := _errorKindNameToValueMap[s]; ok { + return val, nil + } + + if val, ok := _errorKindNameToValueMap[strings.ToLower(s)]; ok { + return val, nil + } + return 0, fmt.Errorf("%s does not belong to errorKind values", s) +} + +// errorKindValues returns all values of the enum +func errorKindValues() []errorKind { + return _errorKindValues +} + +// errorKindStrings returns a slice of all String values of the enum +func errorKindStrings() []string { + strs := make([]string, len(_errorKindNames)) + copy(strs, _errorKindNames) + return strs +} + +// IsAerrorKind returns "true" if the value is listed in the enum definition. "false" otherwise +func (i errorKind) IsAerrorKind() bool { + for _, v := range _errorKindValues { + if i == v { + return true + } + } + return false +} diff --git a/js/modules/k6/experimental/fs/file.go b/js/modules/k6/experimental/fs/file.go new file mode 100644 index 00000000000..d8f5ce90469 --- /dev/null +++ b/js/modules/k6/experimental/fs/file.go @@ -0,0 +1,28 @@ +package fs + +import ( + "path/filepath" +) + +// file is an abstraction for interacting with files. +type file struct { + path string + + // data holds a pointer to the file's data + data []byte +} + +// Stat returns a FileInfo describing the named file. +func (f *file) stat() *FileInfo { + filename := filepath.Base(f.path) + return &FileInfo{Name: filename, Size: len(f.data)} +} + +// FileInfo holds information about a file. +type FileInfo struct { + // Name holds the base name of the file. + Name string `json:"name"` + + // Size holds the size of the file in bytes. + Size int `json:"size"` +} diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go new file mode 100644 index 00000000000..2e9be25d311 --- /dev/null +++ b/js/modules/k6/experimental/fs/module.go @@ -0,0 +1,175 @@ +// Package fs provides a k6 module that allows users to interact with files from the +// local filesystem. +package fs + +import ( + "errors" + "fmt" + + "github.com/dop251/goja" + "go.k6.io/k6/js/common" + "go.k6.io/k6/js/modules" + "go.k6.io/k6/js/promises" + "go.k6.io/k6/lib/fsext" +) + +type ( + // RootModule is the global module instance that will create instances of our + // module for each VU. + RootModule struct { + cache *cache + } + + // ModuleInstance represents an instance of the fs module for a single VU. + ModuleInstance struct { + vu modules.VU + cache *cache + } +) + +var ( + _ modules.Module = &RootModule{} + _ modules.Instance = &ModuleInstance{} +) + +// New returns a pointer to a new [RootModule] instance. +func New() *RootModule { + return &RootModule{ + cache: &cache{}, + } +} + +// NewModuleInstance implements the modules.Module interface and returns a new +// instance of our module for the given VU. +func (rm *RootModule) NewModuleInstance(vu modules.VU) modules.Instance { + return &ModuleInstance{vu: vu, cache: rm.cache} +} + +// Exports implements the modules.Module interface and returns the exports of +// our module. +func (mi *ModuleInstance) Exports() modules.Exports { + return modules.Exports{ + Named: map[string]any{ + "open": mi.Open, + }, + } +} + +// Open opens a file and returns a promise that will resolve to a [File] instance +func (mi *ModuleInstance) Open(path goja.Value) *goja.Promise { + promise, resolve, reject := promises.New(mi.vu) + + // Files can only be opened in the init context. + if mi.vu.State() != nil { + reject(newFsError(ForbiddenError, "open() failed; reason: opening a file in the VU context is forbidden")) + return promise + } + + if common.IsNullish(path) { + reject(newFsError(TypeError, "open() failed; reason: path cannot be null or undefined")) + return promise + } + + // Obtain the underlying path string from the JS value. + pathStr := path.String() + if pathStr == "" { + reject(newFsError(TypeError, "open() failed; reason: path cannot be empty")) + return promise + } + + go func() { + file, err := mi.openImpl(pathStr) + if err != nil { + reject(err) + return + } + + resolve(file) + }() + + return promise +} + +func (mi *ModuleInstance) openImpl(path string) (*File, error) { + initEnv := mi.vu.InitEnv() + + // We resolve the path relative to the entrypoint script, as opposed to + // the current working directory (the k6 command is called from). + // + // This is done on purpose, although it diverges in some respect with + // how files are handled in different k6 contexts, so that we cater to + // and intuitive user experience. + // + // See #2781 and #2674. + path = fsext.Abs(initEnv.CWD.Path, path) + + fs, ok := initEnv.FileSystems["file"] + if !ok { + common.Throw(mi.vu.Runtime(), errors.New("open() failed; reason: unable to access the file system")) + } + + if exists, err := fsext.Exists(fs, path); err != nil { + return nil, fmt.Errorf("open() failed, unable to verify if %q exists; reason: %w", path, err) + } else if !exists { + return nil, newFsError(NotFoundError, fmt.Sprintf("no such file or directory %q", path)) + } + + if isDir, err := fsext.IsDir(fs, path); err != nil { + return nil, fmt.Errorf("open() failed, unable to verify if %q is a directory; reason: %w", path, err) + } else if isDir { + return nil, newFsError( + InvalidResourceError, + fmt.Sprintf("cannot open %q: opening a directory is not supported", path), + ) + } + + data, err := mi.cache.open(path, fs) + if err != nil { + return nil, err + } + + return &File{ + Path: path, + file: file{ + path: path, + data: data, + }, + vu: mi.vu, + registry: mi.cache, + }, nil +} + +// File represents a file and exposes methods to interact with it. +// +// It is a wrapper around the [file] struct, which is meant to be directly +// exposed to the JS runtime. +type File struct { + // Path holds the name of the file, as presented to [Open]. + Path string `json:"path"` + + // file contains the actual implementation for the file system. + file + + // vu holds a reference to the VU this file is associated with. + // + // We need this to be able to access the VU's runtime, and produce + // promises that are handled by the VU's runtime. + vu modules.VU + + // registry holds a pointer to the file registry this file is associated + // with. That way we are able to close the file when it's not needed + // anymore. + registry *cache +} + +// Stat returns a promise that will resolve to a [FileInfo] instance describing +// the file. +func (f *File) Stat() *goja.Promise { + promise, resolve, _ := promises.New(f.vu) + + go func() { + resolve(f.file.stat()) + }() + + return promise +} diff --git a/js/modules/k6/experimental/fs/module_test.go b/js/modules/k6/experimental/fs/module_test.go new file mode 100644 index 00000000000..4dbe63dada7 --- /dev/null +++ b/js/modules/k6/experimental/fs/module_test.go @@ -0,0 +1,359 @@ +package fs + +import ( + "fmt" + "net/url" + "path/filepath" + "testing" + + "github.com/spf13/afero" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.k6.io/k6/js/compiler" + "go.k6.io/k6/js/modulestest" + "go.k6.io/k6/lib" + "go.k6.io/k6/lib/fsext" + "go.k6.io/k6/metrics" +) + +func TestOpen(t *testing.T) { + t.Parallel() + + t.Run("opening existing file should succeed", func(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + openPath string + wantPath string + }{ + { + name: "open absolute path", + openPath: fsext.FilePathSeparator + "bonjour.txt", + wantPath: fsext.FilePathSeparator + "bonjour.txt", + }, + { + name: "open relative path", + openPath: filepath.Join(".", fsext.FilePathSeparator, "bonjour.txt"), + wantPath: fsext.FilePathSeparator + "bonjour.txt", + }, + { + name: "open path with ..", + openPath: fsext.FilePathSeparator + "dir" + fsext.FilePathSeparator + ".." + fsext.FilePathSeparator + "bonjour.txt", + wantPath: fsext.FilePathSeparator + "bonjour.txt", + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + fs := newTestFs(t, func(fs afero.Fs) error { + fileErr := afero.WriteFile(fs, tt.wantPath, []byte("Bonjour, le monde"), 0o644) + if fileErr != nil { + return fileErr + } + + return fs.Mkdir(fsext.FilePathSeparator+"dir", 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + runtime.VU.InitEnvField.CWD = &url.URL{Scheme: "file", Path: fsext.FilePathSeparator} + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + let file; + try { + file = await fs.open(%q) + } catch (err) { + throw "unexpected error: " + err + } + + if (file.path !== %q) { + throw 'unexpected file path ' + file.path + '; expected %q'; + } + `, tt.openPath, tt.wantPath, tt.wantPath))) + + assert.NoError(t, err) + }) + } + }) + + t.Run("opening file in VU context should fail", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + runtime.MoveToVUContext(&lib.State{ + Tags: lib.NewVUStateTags(metrics.NewRegistry().RootTagSet().With("tag-vu", "mytag")), + }) + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(` + try { + const file = await fs.open('bonjour.txt') + throw 'unexpected promise resolution with result: ' + file; + } catch (err) { + if (err.name !== 'ForbiddenError') { + throw 'unexpected error: ' + err + } + } + + `)) + + assert.NoError(t, err) + }) + + t.Run("calling open without providing a path should fail", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(` + let file; + + try { + file = await fs.open() + throw 'unexpected promise resolution with result: ' + file; + } catch (err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err + } + } + + try { + file = await fs.open(null) + throw 'unexpected promise resolution with result: ' + file; + } catch (err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err + } + } + + try { + file = await fs.open(undefined) + throw 'unexpected promise resolution with result: ' + file; + } catch (err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err + } + } + `)) + + assert.NoError(t, err) + }) + + t.Run("opening directory should fail", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testDirPath := fsext.FilePathSeparator + "dir" + fs := newTestFs(t, func(fs afero.Fs) error { + return fs.Mkdir(testDirPath, 0o644) + }) + + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + try { + const file = await fs.open(%q) + throw 'unexpected promise resolution with result: ' + res + } catch (err) { + if (err.name !== 'InvalidResourceError') { + throw 'unexpected error: ' + err + } + } + `, testDirPath))) + + assert.NoError(t, err) + }) + + t.Run("opening non existing file should fail", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(` + try { + const file = await fs.open('doesnotexist.txt') + throw 'unexpected promise resolution with result: ' + res + } catch (err) { + if (err.name !== 'NotFoundError') { + throw 'unexpected error: ' + err + } + } + `)) + + assert.NoError(t, err) + }) +} + +func TestFile(t *testing.T) { + t.Parallel() + + t.Run("stat method should succeed", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + "bonjour.txt" + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("Bonjour, le monde"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + const file = await fs.open(%q) + const info = await file.stat() + + if (info.name !== 'bonjour.txt') { + throw 'unexpected file name ' + info.name + '; expected \'bonjour.txt\''; + } + + if (info.size !== 17) { + throw 'unexpected file size ' + info.size + '; expected 17'; + } + `, testFilePath))) + + assert.NoError(t, err) + }) +} + +func TestOpenImpl(t *testing.T) { + t.Parallel() + + t.Run("should panic if the file system is not available", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + delete(runtime.VU.InitEnvField.FileSystems, "file") + + mi := &ModuleInstance{ + vu: runtime.VU, + cache: &cache{}, + } + + assert.Panics(t, func() { + //nolint:errcheck,gosec + mi.openImpl("bonjour.txt") + }) + }) + + t.Run("should return an error if the file does not exist", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + mi := &ModuleInstance{ + vu: runtime.VU, + cache: &cache{}, + } + + _, err = mi.openImpl("bonjour.txt") + assert.Error(t, err) + var fsError *fsError + assert.ErrorAs(t, err, &fsError) + assert.Equal(t, NotFoundError, fsError.kind) + }) + + t.Run("should return an error if the path is a directory", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + fs := newTestFs(t, func(fs afero.Fs) error { + return fs.Mkdir("/dir", 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + mi := &ModuleInstance{ + vu: runtime.VU, + cache: &cache{}, + } + + _, err = mi.openImpl("/dir") + assert.Error(t, err) + var fsError *fsError + assert.ErrorAs(t, err, &fsError) + assert.Equal(t, InvalidResourceError, fsError.kind) + }) + + t.Run("path is resolved relative to the entrypoint script", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, "/bonjour.txt", []byte("Bonjour, le monde"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + runtime.VU.InitEnvField.CWD = &url.URL{Scheme: "file", Path: "/dir"} + + mi := &ModuleInstance{ + vu: runtime.VU, + cache: &cache{}, + } + + _, err = mi.openImpl("../bonjour.txt") + assert.NoError(t, err) + }) +} + +const initGlobals = ` + globalThis.fs = require("k6/experimental/fs"); +` + +func newConfiguredRuntime(t testing.TB) (*modulestest.Runtime, error) { + runtime := modulestest.NewRuntime(t) + + err := runtime.SetupModuleSystem(map[string]interface{}{"k6/experimental/fs": New()}, nil, compiler.New(runtime.VU.InitEnv().Logger)) + if err != nil { + return nil, err + } + + // Set up the VU environment with an in-memory filesystem and a CWD of "/". + runtime.VU.InitEnvField.FileSystems = map[string]fsext.Fs{ + "file": fsext.NewMemMapFs(), + } + runtime.VU.InitEnvField.CWD = &url.URL{Scheme: "file"} + + // Ensure the `fs` module is available in the VU's runtime. + _, err = runtime.VU.Runtime().RunString(initGlobals) + + return runtime, err +} + +// newTestFs is a helper function that creates a new in-memory file system and calls the provided +// function with it. The provided function is expected to use the file system to create files and +// directories. +func newTestFs(t *testing.T, fn func(fs afero.Fs) error) afero.Fs { + t.Helper() + + fs := afero.NewMemMapFs() + + err := fn(fs) + if err != nil { + t.Fatal(err) + } + + return fs +} + +// wrapInAsyncLambda is a helper function that wraps the provided input in an async lambda. This +// makes the use of `await` statements in the input possible. +func wrapInAsyncLambda(input string) string { + // This makes it possible to use `await` freely on the "top" level + return "(async () => {\n " + input + "\n })()" +} From e4ce7b5f568727570638f6e0a32048064c7f33cc Mon Sep 17 00:00:00 2001 From: oleiade Date: Wed, 5 Jul 2023 11:11:31 +0200 Subject: [PATCH 02/18] Export `fs` module as `k6/experimental/fs` --- js/jsmodules.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/js/jsmodules.go b/js/jsmodules.go index f5ec36a0d81..b19a1e74aeb 100644 --- a/js/jsmodules.go +++ b/js/jsmodules.go @@ -8,6 +8,7 @@ import ( "go.k6.io/k6/js/modules/k6/data" "go.k6.io/k6/js/modules/k6/encoding" "go.k6.io/k6/js/modules/k6/execution" + "go.k6.io/k6/js/modules/k6/experimental/fs" "go.k6.io/k6/js/modules/k6/experimental/tracing" "go.k6.io/k6/js/modules/k6/grpc" "go.k6.io/k6/js/modules/k6/html" @@ -38,6 +39,7 @@ func getInternalJSModules() map[string]interface{} { "k6/experimental/timers": timers.New(), "k6/experimental/tracing": tracing.New(), "k6/experimental/browser": browser.New(), + "k6/experimental/fs": fs.New(), "k6/net/grpc": grpc.New(), "k6/html": html.New(), "k6/http": http.New(), From 7125929760749301a6e75a69125752fb7022f944 Mon Sep 17 00:00:00 2001 From: oleiade Date: Wed, 5 Jul 2023 11:12:04 +0200 Subject: [PATCH 03/18] Add example of `k6/experimental/fs` module --- examples/experimental/fs/bonjour.txt | 1 + examples/experimental/fs/fs.js | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+) create mode 100644 examples/experimental/fs/bonjour.txt create mode 100644 examples/experimental/fs/fs.js diff --git a/examples/experimental/fs/bonjour.txt b/examples/experimental/fs/bonjour.txt new file mode 100644 index 00000000000..589326a33ee --- /dev/null +++ b/examples/experimental/fs/bonjour.txt @@ -0,0 +1 @@ +Bonjour, tout le monde! \ No newline at end of file diff --git a/examples/experimental/fs/fs.js b/examples/experimental/fs/fs.js new file mode 100644 index 00000000000..d7347c97007 --- /dev/null +++ b/examples/experimental/fs/fs.js @@ -0,0 +1,20 @@ +import { open } from "k6/experimental/fs"; + +export const options = { + vus: 100, + iterations: 1000, +}; + +// As k6 does not support asynchronous code in the init context, yet, we need to +// use a top-level async function to be able to use the `await` keyword. +let file; +(async function () { + file = await open("bonjour.txt"); +})(); + +export default async function () { + const fileinfo = await file.stat(); + if (fileinfo.name != "bonjour.txt") { + throw new Error("Unexpected file name"); + } +} From 80ada7186f8ebb926203ade8527819597bd4e3d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20Crevon?= Date: Mon, 6 Nov 2023 17:26:25 +0100 Subject: [PATCH 04/18] [fs] Add `file.read` and `file.seek` methods to the `fs` module (2/3) (#3309) --- examples/experimental/fs/fs.js | 38 +- js/modules/k6/experimental/fs/errors.go | 3 + js/modules/k6/experimental/fs/errors_gen.go | 12 +- js/modules/k6/experimental/fs/file.go | 114 ++++++ js/modules/k6/experimental/fs/file_test.go | 258 ++++++++++++++ js/modules/k6/experimental/fs/module.go | 152 ++++++++ js/modules/k6/experimental/fs/module_test.go | 354 ++++++++++++++++++- 7 files changed, 915 insertions(+), 16 deletions(-) create mode 100644 js/modules/k6/experimental/fs/file_test.go diff --git a/examples/experimental/fs/fs.js b/examples/experimental/fs/fs.js index d7347c97007..5ff3d0a00b3 100644 --- a/examples/experimental/fs/fs.js +++ b/examples/experimental/fs/fs.js @@ -1,20 +1,52 @@ -import { open } from "k6/experimental/fs"; +import { open, SeekMode } from "k6/experimental/fs"; export const options = { vus: 100, iterations: 1000, }; -// As k6 does not support asynchronous code in the init context, yet, we need to -// use a top-level async function to be able to use the `await` keyword. +// k6 doesn't support async in the init context. We use a top-level async function for `await`. +// +// Each Virtual User gets its own `file` copy. +// So, operations like `seek` or `read` won't impact other VUs. let file; (async function () { file = await open("bonjour.txt"); })(); export default async function () { + // About information about the file const fileinfo = await file.stat(); if (fileinfo.name != "bonjour.txt") { throw new Error("Unexpected file name"); } + + const buffer = new Uint8Array(4); + + let totalBytesRead = 0; + while (true) { + // Read into the buffer + const bytesRead = await file.read(buffer); + if (bytesRead == null) { + // EOF + break; + } + + // Do something useful with the content of the buffer + + totalBytesRead += bytesRead; + + // If bytesRead is less than the buffer size, we've read the whole file + if (bytesRead < buffer.byteLength) { + break; + } + } + + // Check that we read the expected number of bytes + if (totalBytesRead != fileinfo.size) { + throw new Error("Unexpected number of bytes read"); + } + + // Seek back to the beginning of the file + await file.seek(0, SeekMode.Start); } diff --git a/js/modules/k6/experimental/fs/errors.go b/js/modules/k6/experimental/fs/errors.go index c31a52c72c1..95fcb9e0167 100644 --- a/js/modules/k6/experimental/fs/errors.go +++ b/js/modules/k6/experimental/fs/errors.go @@ -34,6 +34,9 @@ const ( // TypeError is emitted when an incorrect type has been used. TypeError + + // EOFError is emitted when the end of a file has been reached. + EOFError ) // fsError represents a custom error object emitted by the fs module. diff --git a/js/modules/k6/experimental/fs/errors_gen.go b/js/modules/k6/experimental/fs/errors_gen.go index 7ffc4a49d3c..feb369058af 100644 --- a/js/modules/k6/experimental/fs/errors_gen.go +++ b/js/modules/k6/experimental/fs/errors_gen.go @@ -7,11 +7,11 @@ import ( "strings" ) -const _errorKindName = "NotFoundErrorInvalidResourceErrorForbiddenErrorTypeError" +const _errorKindName = "NotFoundErrorInvalidResourceErrorForbiddenErrorTypeErrorEOFError" -var _errorKindIndex = [...]uint8{0, 13, 33, 47, 56} +var _errorKindIndex = [...]uint8{0, 13, 33, 47, 56, 64} -const _errorKindLowerName = "notfounderrorinvalidresourceerrorforbiddenerrortypeerror" +const _errorKindLowerName = "notfounderrorinvalidresourceerrorforbiddenerrortypeerroreoferror" func (i errorKind) String() string { i -= 1 @@ -29,9 +29,10 @@ func _errorKindNoOp() { _ = x[InvalidResourceError-(2)] _ = x[ForbiddenError-(3)] _ = x[TypeError-(4)] + _ = x[EOFError-(5)] } -var _errorKindValues = []errorKind{NotFoundError, InvalidResourceError, ForbiddenError, TypeError} +var _errorKindValues = []errorKind{NotFoundError, InvalidResourceError, ForbiddenError, TypeError, EOFError} var _errorKindNameToValueMap = map[string]errorKind{ _errorKindName[0:13]: NotFoundError, @@ -42,6 +43,8 @@ var _errorKindNameToValueMap = map[string]errorKind{ _errorKindLowerName[33:47]: ForbiddenError, _errorKindName[47:56]: TypeError, _errorKindLowerName[47:56]: TypeError, + _errorKindName[56:64]: EOFError, + _errorKindLowerName[56:64]: EOFError, } var _errorKindNames = []string{ @@ -49,6 +52,7 @@ var _errorKindNames = []string{ _errorKindName[13:33], _errorKindName[33:47], _errorKindName[47:56], + _errorKindName[56:64], } // errorKindString retrieves an enum value from the enum constants string name. diff --git a/js/modules/k6/experimental/fs/file.go b/js/modules/k6/experimental/fs/file.go index d8f5ce90469..cdebe2a6384 100644 --- a/js/modules/k6/experimental/fs/file.go +++ b/js/modules/k6/experimental/fs/file.go @@ -1,7 +1,11 @@ package fs import ( + "io" "path/filepath" + "sync/atomic" + + "go.k6.io/k6/lib" ) // file is an abstraction for interacting with files. @@ -10,6 +14,13 @@ type file struct { // data holds a pointer to the file's data data []byte + + // offset holds the current offset in the file + // + // TODO: using an atomic here does not guarantee ordering of reads and seeks, and leaves + // the behavior not strictly defined. This is something we might want to address in the future, and + // is tracked as part of #3433. + offset atomic.Int64 } // Stat returns a FileInfo describing the named file. @@ -26,3 +37,106 @@ type FileInfo struct { // Size holds the size of the file in bytes. Size int `json:"size"` } + +// Read reads up to len(into) bytes into the provided byte slice. +// +// It returns the number of bytes read (0 <= n <= len(into)) and any error +// encountered. +// +// If the end of the file has been reached, it returns EOFError. +func (f *file) Read(into []byte) (n int, err error) { + currentOffset := f.offset.Load() + fileSize := f.size() + + // Check if we have reached the end of the file + if currentOffset == fileSize { + return 0, newFsError(EOFError, "EOF") + } + + // Calculate the effective new offset + targetOffset := currentOffset + int64(len(into)) + newOffset := lib.Min(targetOffset, fileSize) + + // Read the data into the provided slice, and update + // the offset accordingly + n = copy(into, f.data[currentOffset:newOffset]) + f.offset.Store(newOffset) + + // If we've reached or surpassed the end, set the error to EOF + if targetOffset > fileSize { + err = newFsError(EOFError, "EOF") + } + + return n, err +} + +// Ensure that `file` implements the io.Reader interface. +var _ io.Reader = (*file)(nil) + +// Seek sets the offset for the next operation on the file, under the mode given by `whence`. +// +// `offset` indicates the number of bytes to move the offset. Based on +// the `whence` parameter, the offset is set relative to the start, +// current offset or end of the file. +// +// When using SeekModeStart, the offset must be positive. +// Negative offsets are allowed when using `SeekModeCurrent` or `SeekModeEnd`. +func (f *file) Seek(offset int64, whence SeekMode) (int64, error) { + startingOffset := f.offset.Load() + + newOffset := startingOffset + switch whence { + case SeekModeStart: + if offset < 0 { + return 0, newFsError(TypeError, "offset cannot be negative when using SeekModeStart") + } + + newOffset = offset + case SeekModeCurrent: + newOffset += offset + case SeekModeEnd: + if offset > 0 { + return 0, newFsError(TypeError, "offset cannot be positive when using SeekModeEnd") + } + + newOffset = f.size() + offset + default: + return 0, newFsError(TypeError, "invalid seek mode") + } + + if newOffset < 0 { + return 0, newFsError(TypeError, "seeking before start of file") + } + + if newOffset > f.size() { + return 0, newFsError(TypeError, "seeking beyond end of file") + } + + // Update the file instance's offset to the new selected position + f.offset.Store(newOffset) + + return newOffset, nil +} + +var _ io.Seeker = (*file)(nil) + +// SeekMode is used to specify the seek mode when seeking in a file. +type SeekMode = int + +const ( + // SeekModeStart sets the offset relative to the start of the file. + SeekModeStart SeekMode = 0 + + // SeekModeCurrent seeks relative to the current offset. + SeekModeCurrent = 1 + + // SeekModeEnd seeks relative to the end of the file. + // + // When using this mode the seek operation will move backwards from + // the end of the file. + SeekModeEnd = 2 +) + +func (f *file) size() int64 { + return int64(len(f.data)) +} diff --git a/js/modules/k6/experimental/fs/file_test.go b/js/modules/k6/experimental/fs/file_test.go new file mode 100644 index 00000000000..c93554d0ecf --- /dev/null +++ b/js/modules/k6/experimental/fs/file_test.go @@ -0,0 +1,258 @@ +package fs + +import ( + "bytes" + "errors" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestFileImpl(t *testing.T) { + t.Parallel() + + t.Run("read", func(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + into []byte + fileData []byte + offset int64 + wantInto []byte + wantN int + wantErr errorKind + }{ + { + name: "reading the entire file into a buffer fitting the whole file should succeed", + into: make([]byte, 5), + fileData: []byte("hello"), + offset: 0, + wantInto: []byte("hello"), + wantN: 5, + wantErr: 0, // No error expected + }, + { + name: "reading a file larger than the provided buffer should succeed", + into: make([]byte, 3), + fileData: []byte("hello"), + offset: 0, + wantInto: []byte("hel"), + wantN: 3, + wantErr: 0, // No error expected + }, + { + name: "reading a file larger than the provided buffer at an offset should succeed", + into: make([]byte, 3), + fileData: []byte("hello"), + offset: 2, + wantInto: []byte("llo"), + wantN: 3, + wantErr: 0, // No error expected + }, + { + name: "reading file data into a zero sized buffer should succeed", + into: []byte{}, + fileData: []byte("hello"), + offset: 0, + wantInto: []byte{}, + wantN: 0, + wantErr: 0, // No error expected + }, + { + name: "reading past the end of the file should fill the buffer and fail with EOF", + into: make([]byte, 10), + fileData: []byte("hello"), + offset: 0, + wantInto: []byte{'h', 'e', 'l', 'l', 'o', 0, 0, 0, 0, 0}, + wantN: 5, + wantErr: EOFError, + }, + { + name: "reading into a prefilled buffer overrides its content", + into: []byte("world!"), + fileData: []byte("hello"), + offset: 0, + wantInto: []byte("hello!"), + wantN: 5, + wantErr: EOFError, + }, + { + name: "reading an empty file should fail with EOF", + into: make([]byte, 10), + fileData: []byte{}, + offset: 0, + wantInto: []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + wantN: 0, + wantErr: EOFError, + }, + { + name: "reading from the end of a file should fail with EOF", + into: make([]byte, 10), + // Note that the offset is larger than the file size + fileData: []byte("hello"), + offset: 5, + wantInto: []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0}, + wantN: 0, + wantErr: EOFError, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f := &file{ + path: "", + data: tc.fileData, + } + f.offset.Store(tc.offset) + + gotN, err := f.Read(tc.into) + + // Cast the error to your custom error type to access its kind + var gotErr errorKind + if err != nil { + var fsErr *fsError + ok := errors.As(err, &fsErr) + if !ok { + t.Fatalf("unexpected error type: got %T, want %T", err, &fsError{}) + } + gotErr = fsErr.kind + } + + if gotN != tc.wantN || gotErr != tc.wantErr { + t.Errorf("Read() = %d, %v, want %d, %v", gotN, gotErr, tc.wantN, tc.wantErr) + } + + if !bytes.Equal(tc.into, tc.wantInto) { + t.Errorf("Read() into = %v, want %v", tc.into, tc.wantInto) + } + }) + } + }) + + t.Run("seek", func(t *testing.T) { + t.Parallel() + + type args struct { + offset int64 + whence SeekMode + } + + // The test file is 100 bytes long + tests := []struct { + name string + fileOffset int64 + args args + wantOffset int64 + wantError bool + }{ + { + name: "seek using SeekModeStart within file bounds should succeed", + fileOffset: 0, + args: args{50, SeekModeStart}, + wantOffset: 50, + wantError: false, + }, + { + name: "seek using SeekModeStart beyond file boundaries should fail", + fileOffset: 0, + args: args{150, SeekModeStart}, + wantOffset: 0, + wantError: true, + }, + { + name: "seek using SeekModeStart and a negative offset should fail", + fileOffset: 0, + args: args{-50, SeekModeStart}, + wantOffset: 0, + wantError: true, + }, + { + name: "seek using SeekModeCurrent within file bounds at offset 0 should succeed", + fileOffset: 0, + args: args{10, SeekModeCurrent}, + wantOffset: 10, + wantError: false, + }, + { + name: "seek using SeekModeCurrent within file bounds at non-zero offset should succeed", + fileOffset: 20, + args: args{10, SeekModeCurrent}, + wantOffset: 30, + wantError: false, + }, + { + name: "seek using SeekModeCurrent beyond file boundaries should fail", + fileOffset: 20, + args: args{100, SeekModeCurrent}, + wantOffset: 20, + wantError: true, + }, + { + name: "seek using SeekModeCurrent and a negative offset should succeed", + fileOffset: 20, + args: args{-10, SeekModeCurrent}, + wantOffset: 10, + wantError: false, + }, + { + name: "seek using SeekModeCurrent and an out of bound negative offset should fail", + fileOffset: 20, + args: args{-40, SeekModeCurrent}, + wantOffset: 20, + wantError: true, + }, + { + name: "seek using SeekModeEnd within file bounds should succeed", + fileOffset: 20, + args: args{-20, SeekModeEnd}, + wantOffset: 80, + wantError: false, + }, + { + name: "seek using SeekModeEnd beyond file start should fail", + fileOffset: 20, + args: args{-110, SeekModeEnd}, + wantOffset: 20, + wantError: true, // File is 100 bytes long + }, + { + name: "seek using SeekModeEnd and a positive offset should fail", + fileOffset: 20, + args: args{10, SeekModeEnd}, + wantOffset: 20, + wantError: true, + }, + { + name: "seek with invalid whence should fail", + fileOffset: 0, + args: args{10, SeekMode(42)}, + wantOffset: 0, + wantError: true, + }, + } + + for _, tt := range tests { + tt := tt + + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + f := &file{data: make([]byte, 100)} + f.offset.Store(tt.fileOffset) + + got, err := f.Seek(tt.args.offset, tt.args.whence) + if tt.wantError { + assert.Error(t, err, tt.name) + } else { + assert.NoError(t, err, tt.name) + assert.Equal(t, tt.wantOffset, got, tt.name) + } + }) + } + }) +} diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index 2e9be25d311..df060b4aa6c 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -51,6 +51,11 @@ func (mi *ModuleInstance) Exports() modules.Exports { return modules.Exports{ Named: map[string]any{ "open": mi.Open, + "SeekMode": map[string]any{ + "Start": SeekModeStart, + "Current": SeekModeCurrent, + "End": SeekModeEnd, + }, }, } } @@ -173,3 +178,150 @@ func (f *File) Stat() *goja.Promise { return promise } + +// Read the file's content, and writes it into the provided Uint8Array. +// +// Resolves to either the number of bytes read during the operation +// or EOF (null) if there was nothing more to read. +// +// It is possible for a read to successfully return with 0 bytes. +// This does not indicate EOF. +func (f *File) Read(into goja.Value) *goja.Promise { + promise, resolve, reject := f.vu.Runtime().NewPromise() + + if common.IsNullish(into) { + reject(newFsError(TypeError, "read() failed; reason: into cannot be null or undefined")) + return promise + } + + // We expect the into argument to be a `Uint8Array` instance + + // intoObj := into.ToObject(f.vu.Runtime()) + // uint8ArrayConstructor := f.vu.Runtime().Get("Uint8Array") + // if isUint8Array := intoObj.Get("constructor").SameAs(uint8ArrayConstructor); !isUint8Array { + intoObj := into.ToObject(f.vu.Runtime()) + if !isUint8Array(f.vu.Runtime(), intoObj) { + reject(newFsError(TypeError, "read() failed; reason: into argument must be a Uint8Array")) + return promise + } + + // Obtain the underlying ArrayBuffer from the Uint8Array + ab, ok := intoObj.Get("buffer").Export().(goja.ArrayBuffer) + if !ok { + reject(newFsError(TypeError, "read() failed; reason: into argument must be a Uint8Array")) + return promise + } + + // To avoid concurrency linked to modifying the runtime's `into` buffer from multiple + // goroutines we make sure to work on a separate copy, and will copy the bytes back + // into the runtime's `into` buffer once the promise is resolved. + intoBytes := ab.Bytes() + buffer := make([]byte, len(intoBytes)) + + // We register a callback to be executed by the VU's runtime. + // This ensures that the modification of the JS runtime's `into` buffer + // occurs on the main thread, during the promise's resolution. + callback := f.vu.RegisterCallback() + go func() { + n, readErr := f.file.Read(buffer) + callback(func() error { + _ = copy(intoBytes[0:n], buffer) + + // Read was successful, resolve early with the number of + // bytes read. + if readErr == nil { + resolve(n) + return nil + } + + // The [file.Read] method will return an EOFError as soon as it reached + // the end of the file. + // + // However, following deno's behavior, we express + // EOF to users by returning null, when and only when there aren't any + // more bytes to read. + var fsErr *fsError + isFSErr := errors.As(readErr, &fsErr) + + if !isFSErr { + reject(readErr) + return nil + } + + if fsErr.kind == EOFError && n == 0 { + resolve(goja.Null()) + } else { + resolve(n) + } + + return nil + }) + }() + + return promise +} + +// Seek seeks to the given `offset` in the file, under the given `whence` mode. +// +// The returned promise resolves to the new `offset` (position) within the file, which +// is expressed in bytes from the selected start, current, or end position depending +// the provided `whence`. +func (f *File) Seek(offset goja.Value, whence goja.Value) *goja.Promise { + promise, resolve, reject := f.vu.Runtime().NewPromise() + + if common.IsNullish(offset) { + reject(newFsError(TypeError, "seek() failed; reason: the offset argument cannot be null or undefined")) + return promise + } + + var intOffset int64 + if err := f.vu.Runtime().ExportTo(offset, &intOffset); err != nil { + reject(newFsError(TypeError, "seek() failed; reason: the offset argument cannot be interpreted as integer")) + return promise + } + + if common.IsNullish(whence) { + reject(newFsError(TypeError, "seek() failed; reason: the whence argument cannot be null or undefined")) + return promise + } + + var intWhence int64 + if err := f.vu.Runtime().ExportTo(whence, intWhence); err != nil { + reject(newFsError(TypeError, "seek() failed; reason: the whence argument cannot be interpreted as integer")) + return promise + } + + seekMode := SeekMode(intWhence) + switch seekMode { + case SeekModeStart, SeekModeCurrent, SeekModeEnd: + // Valid modes, do nothing. + default: + reject(newFsError(TypeError, "seek() failed; reason: the whence argument must be a SeekMode")) + return promise + } + + callback := f.vu.RegisterCallback() + go func() { + newOffset, err := f.file.Seek(intOffset, seekMode) + callback(func() error { + if err != nil { + reject(err) + return err + } + + resolve(newOffset) + return nil + }) + }() + + return promise +} + +func isUint8Array(rt *goja.Runtime, o *goja.Object) bool { + uint8ArrayConstructor := rt.Get("Uint8Array") + if isUint8Array := o.Get("constructor").SameAs(uint8ArrayConstructor); !isUint8Array { + return false + } + + return true +} diff --git a/js/modules/k6/experimental/fs/module_test.go b/js/modules/k6/experimental/fs/module_test.go index 4dbe63dada7..30906e8ceda 100644 --- a/js/modules/k6/experimental/fs/module_test.go +++ b/js/modules/k6/experimental/fs/module_test.go @@ -16,6 +16,8 @@ import ( "go.k6.io/k6/metrics" ) +const testFileName = "bonjour.txt" + func TestOpen(t *testing.T) { t.Parallel() @@ -29,18 +31,18 @@ func TestOpen(t *testing.T) { }{ { name: "open absolute path", - openPath: fsext.FilePathSeparator + "bonjour.txt", - wantPath: fsext.FilePathSeparator + "bonjour.txt", + openPath: fsext.FilePathSeparator + testFileName, + wantPath: fsext.FilePathSeparator + testFileName, }, { name: "open relative path", - openPath: filepath.Join(".", fsext.FilePathSeparator, "bonjour.txt"), - wantPath: fsext.FilePathSeparator + "bonjour.txt", + openPath: filepath.Join(".", fsext.FilePathSeparator, testFileName), + wantPath: fsext.FilePathSeparator + testFileName, }, { name: "open path with ..", - openPath: fsext.FilePathSeparator + "dir" + fsext.FilePathSeparator + ".." + fsext.FilePathSeparator + "bonjour.txt", - wantPath: fsext.FilePathSeparator + "bonjour.txt", + openPath: fsext.FilePathSeparator + "dir" + fsext.FilePathSeparator + ".." + fsext.FilePathSeparator + testFileName, + wantPath: fsext.FilePathSeparator + testFileName, }, } @@ -204,7 +206,7 @@ func TestFile(t *testing.T) { runtime, err := newConfiguredRuntime(t) require.NoError(t, err) - testFilePath := fsext.FilePathSeparator + "bonjour.txt" + testFilePath := fsext.FilePathSeparator + testFileName fs := newTestFs(t, func(fs afero.Fs) error { return afero.WriteFile(fs, testFilePath, []byte("Bonjour, le monde"), 0o644) }) @@ -225,6 +227,340 @@ func TestFile(t *testing.T) { assert.NoError(t, err) }) + + t.Run("read in multiple iterations", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("01234"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + const file = await fs.open(%q); + + let fileContent = new Uint8Array(5); + + let bytesRead; + let buffer = new Uint8Array(3); + + bytesRead = await file.read(buffer) + if (bytesRead !== 3) { + throw 'expected read to return 3, got ' + bytesRead + ' instead'; + } + + // We expect the buffer to be filled with the three first + // bytes of the file, namely '012'. + if (buffer[0] !== 48 || buffer[1] !== 49 || buffer[2] !== 50) { + throw 'expected buffer to be [48, 49, 50], got ' + buffer + ' instead'; + } + + fileContent.set(buffer, 0); + + bytesRead = await file.read(buffer) + if (bytesRead !== 2) { + throw 'expected read to return 2, got ' + bytesRead + ' instead'; + } + + // We expect the buffer to hold the two last bytes of the + // file, namely '34', and as we read only two bytes, its last + // one is expected to be untouched from the previous read. + if (buffer[0] !== 51 || buffer[1] !== 52 || buffer[2] !== 50) { + throw 'expected buffer to be [51, 52, 50], got ' + buffer + ' instead'; + } + + fileContent.set(buffer.subarray(0, bytesRead), 3); + + bytesRead = await file.read(buffer) + if (bytesRead !== null) { + throw 'expected read to return null, got ' + bytesRead + ' instead'; + } + + // We expect the buffer to be untouched. + if (buffer[0] !== 51 || buffer[1] !== 52 || buffer[2] !== 50) { + throw 'expected buffer to be [51, 52, 50], got ' + buffer + ' instead'; + } + `, testFilePath))) + + assert.NoError(t, err) + }) + + t.Run("read called when end of file reached should return null and succeed", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + const file = await fs.open(%q); + let buffer = new Uint8Array(3); + + // Reading the whole file should return 3. + let bytesRead = await file.read(buffer); + if (bytesRead !== 3) { + throw 'expected read to return 3, got ' + bytesRead + ' instead'; + } + + // Reading from the end of the file should return null and + // leave the buffer untouched. + bytesRead = await file.read(buffer); + if (bytesRead !== null) { + throw 'expected read to return null got ' + bytesRead + ' instead'; + } + + if (buffer[0] !== 48 || buffer[1] !== 49 || buffer[2] !== 50) { + throw 'expected buffer to be [48, 49, 50], got ' + buffer + ' instead'; + } + `, testFilePath))) + + assert.NoError(t, err) + }) + + t.Run("read called with invalid argument should fail", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("Bonjour, le monde"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + const file = await fs.open(%q); + let bytesRead; + + // No argument should fail with TypeError. + try { + bytesRead = await file.read() + } catch(err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err; + } + } + + // null buffer argument should fail with TypeError. + try { + bytesRead = await file.read(null) + } catch(err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err; + } + } + + // undefined buffer argument should fail with TypeError. + try { + bytesRead = await file.read(undefined) + } catch (err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err; + } + } + + // Invalid typed array argument should fail with TypeError. + try { + bytesRead = await file.read(new Int32Array(5)) + } catch (err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err; + } + } + + // ArrayBuffer argument should fail with TypeError. + try { + bytesRead = await file.read(new ArrayBuffer(5)) + } catch (err) { + if (err.name !== 'TypeError') { + throw 'unexpected error: ' + err; + } + } + `, testFilePath))) + + assert.NoError(t, err) + }) + + // Regression test for [#3309] + // + // [#3309]: https://github.com/grafana/k6/pull/3309#discussion_r1378528010 + t.Run("read with a buffer of the size of the file + 1 should succeed ", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + // file size is 3 + const file = await fs.open(%q); + + // Create a buffer of size fileSize + 1 + let buffer = new Uint8Array(4); + let n = await file.read(buffer) + if (n !== 3) { + throw 'expected read to return 10, got ' + n + ' instead'; + } + + if (buffer[0] !== 48 || buffer[1] !== 49 || buffer[2] !== 50) { + throw 'expected buffer to be [48, 49, 50], got ' + buffer + ' instead'; + } + + if (buffer[3] !== 0) { + throw 'expected buffer to be [48, 49, 50, 0], got ' + buffer + ' instead'; + } + `, testFilePath))) + + assert.NoError(t, err) + }) + + t.Run("read called concurrently and later resolved should safely modify the buffer read into", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + let file = await fs.open(%q); + + let buffer = new Uint8Array(4) + let p1 = file.read(buffer); + let p2 = file.read(buffer); + + await Promise.all([p1, p2]); + `, testFilePath))) + + assert.NoError(t, err) + }) + + t.Run("read shouldn't be affected by buffer changes happening before resolution", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + let file = await fs.open(%q); + + let buffer = new Uint8Array(5); + let p1 = file.read(buffer); + buffer[0] = 3; + + const bufferCopy = buffer; + + await p1; + `, testFilePath))) + + assert.NoError(t, err) + }) + + t.Run("seek with invalid arguments should fail", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs afero.Fs) error { + return afero.WriteFile(fs, testFilePath, []byte("hello"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + const file = await fs.open(%q) + + let newOffset + + // null offset should fail with TypeError. + try { + newOffset = await file.seek(null) + throw "file.seek(null) promise unexpectedly resolved with result: " + newOffset + } catch (err) { + if (err.name !== 'TypeError') { + throw "file.seek(null) rejected with unexpected error: " + err + } + } + + // undefined offset should fail with TypeError. + try { + newOffset = await file.seek(undefined) + throw "file.seek(undefined) promise unexpectedly promise resolved with result: " + newOffset + } catch (err) { + if (err.name !== 'TypeError') { + throw "file.seek(undefined) rejected with unexpected error: " + err + } + } + + // Invalid type offset should fail with TypeError. + try { + newOffset = await file.seek('abc') + throw "file.seek('abc') promise unexpectedly resolved with result: " + newOffset + } catch (err) { + if (err.name !== 'TypeError') { + throw "file.seek('1') rejected with unexpected error: " + err + } + } + + // Negative offset should fail with TypeError. + try { + newOffset = await file.seek(-1) + throw "file.seek(-1) promise unexpectedly resolved with result: " + newOffset + } catch (err) { + if (err.name !== 'TypeError') { + throw "file.seek(-1) rejected with unexpected error: " + err + } + } + + // Invalid type whence should fail with TypeError. + try { + newOffset = await file.seek(1, 'abc') + throw "file.seek(1, 'abc') promise unexpectedly resolved with result: " + newOffset + } catch (err) { + if (err.name !== 'TypeError') { + throw "file.seek(1, 'abc') rejected with unexpected error: " + err + } + } + + // Invalid whence should fail with TypeError. + try { + newOffset = await file.seek(1, -1) + throw "file.seek(1, -1) promise unexpectedly resolved with result: " + newOffset + } catch (err) { + if (err.name !== 'TypeError') { + throw "file.seek(1, -1) rejected with unexpected error: " + err + } + } + `, testFilePath))) + + assert.NoError(t, err) + }) } func TestOpenImpl(t *testing.T) { @@ -244,7 +580,7 @@ func TestOpenImpl(t *testing.T) { assert.Panics(t, func() { //nolint:errcheck,gosec - mi.openImpl("bonjour.txt") + mi.openImpl(testFileName) }) }) @@ -259,7 +595,7 @@ func TestOpenImpl(t *testing.T) { cache: &cache{}, } - _, err = mi.openImpl("bonjour.txt") + _, err = mi.openImpl(testFileName) assert.Error(t, err) var fsError *fsError assert.ErrorAs(t, err, &fsError) From b758f99a10ff09d6b005cde434c1b6acb15c5488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20Crevon?= Date: Tue, 7 Nov 2023 11:02:58 +0100 Subject: [PATCH 05/18] Apply suggestions from code review Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com> --- examples/experimental/fs/fs.js | 1 - js/modules/k6/experimental/fs/module.go | 5 ++--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/examples/experimental/fs/fs.js b/examples/experimental/fs/fs.js index 5ff3d0a00b3..436f94ac543 100644 --- a/examples/experimental/fs/fs.js +++ b/examples/experimental/fs/fs.js @@ -33,7 +33,6 @@ export default async function () { } // Do something useful with the content of the buffer - totalBytesRead += bytesRead; // If bytesRead is less than the buffer size, we've read the whole file diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index df060b4aa6c..faf1b87105f 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -64,9 +64,8 @@ func (mi *ModuleInstance) Exports() modules.Exports { func (mi *ModuleInstance) Open(path goja.Value) *goja.Promise { promise, resolve, reject := promises.New(mi.vu) - // Files can only be opened in the init context. if mi.vu.State() != nil { - reject(newFsError(ForbiddenError, "open() failed; reason: opening a file in the VU context is forbidden")) + reject(newFsError(ForbiddenError, "open() failed; reason: opening a file is allowed only in the Init context")) return promise } @@ -190,7 +189,7 @@ func (f *File) Read(into goja.Value) *goja.Promise { promise, resolve, reject := f.vu.Runtime().NewPromise() if common.IsNullish(into) { - reject(newFsError(TypeError, "read() failed; reason: into cannot be null or undefined")) + reject(newFsError(TypeError, "read() failed; reason: into argument cannot be null or undefined")) return promise } From d4210443ae8404aa600d3ccb2cf11ad22e0c2acd Mon Sep 17 00:00:00 2001 From: oleiade Date: Tue, 7 Nov 2023 17:12:24 +0100 Subject: [PATCH 06/18] Apply suggestions from code review --- js/modules/k6/experimental/fs/cache.go | 12 ++++++------ js/modules/k6/experimental/fs/module.go | 11 ----------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/js/modules/k6/experimental/fs/cache.go b/js/modules/k6/experimental/fs/cache.go index 2602d2821d2..c7b9f41fc76 100644 --- a/js/modules/k6/experimental/fs/cache.go +++ b/js/modules/k6/experimental/fs/cache.go @@ -6,7 +6,7 @@ import ( "path/filepath" "sync" - "github.com/spf13/afero" + "go.k6.io/k6/lib/fsext" ) // cache is a cache of opened files. @@ -40,14 +40,14 @@ type cache struct { // // Parameters: // - filename: The name of the file to be retrieved. This should be a relative or absolute path. -// - fromFs: The filesystem (from the afero package) from which the file should be read if not already cached. +// - fromFs: The filesystem (from the fsext package) from which the file should be read if not already cached. // // Returns: // - A byte slice containing the content of the specified file. // - An error if there's any issue opening or reading the file. If the file content is // successfully cached and returned once, subsequent calls will not produce // file-related errors for the same file, as the cached value will be used. -func (fr *cache) open(filename string, fromFs afero.Fs) (data []byte, err error) { +func (fr *cache) open(filename string, fromFs fsext.Fs) (data []byte, err error) { filename = filepath.Clean(filename) if f, ok := fr.openedFiles.Load(filename); ok { @@ -59,9 +59,9 @@ func (fr *cache) open(filename string, fromFs afero.Fs) (data []byte, err error) return data, nil } - // The underlying afero.Fs.Open method will cache the file content during this + // The underlying fsext.Fs.Open method will cache the file content during this // operation. Which will lead to effectively holding the content of the file in memory twice. - // However, as per #1079, we plan to eventually reduce our dependency on afero, and + // However, as per #1079, we plan to eventually reduce our dependency on fsext, and // expect this issue to be resolved at that point. // TODO: re-evaluate opening from the FS this once #1079 is resolved. f, err := fromFs.Open(filename) @@ -77,7 +77,7 @@ func (fr *cache) open(filename string, fromFs afero.Fs) (data []byte, err error) data, err = io.ReadAll(f) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to read the content of file %s: %w", filename, err) } fr.openedFiles.Store(filename, data) diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index faf1b87105f..cf5964adc87 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -193,11 +193,6 @@ func (f *File) Read(into goja.Value) *goja.Promise { return promise } - // We expect the into argument to be a `Uint8Array` instance - - // intoObj := into.ToObject(f.vu.Runtime()) - // uint8ArrayConstructor := f.vu.Runtime().Get("Uint8Array") - // if isUint8Array := intoObj.Get("constructor").SameAs(uint8ArrayConstructor); !isUint8Array { intoObj := into.ToObject(f.vu.Runtime()) if !isUint8Array(f.vu.Runtime(), intoObj) { reject(newFsError(TypeError, "read() failed; reason: into argument must be a Uint8Array")) @@ -233,12 +228,6 @@ func (f *File) Read(into goja.Value) *goja.Promise { return nil } - // The [file.Read] method will return an EOFError as soon as it reached - // the end of the file. - // - // However, following deno's behavior, we express - // EOF to users by returning null, when and only when there aren't any - // more bytes to read. var fsErr *fsError isFSErr := errors.As(readErr, &fsErr) From f45bbb1e734353368f40a0d697e4fafdf2603885 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20Crevon?= Date: Wed, 8 Nov 2023 10:05:59 +0100 Subject: [PATCH 07/18] Apply suggestions from code review Co-authored-by: Mihail Stoykov <312246+mstoykov@users.noreply.github.com> --- js/modules/k6/experimental/fs/cache.go | 2 +- js/modules/k6/experimental/fs/file.go | 2 +- js/modules/k6/experimental/fs/module.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/js/modules/k6/experimental/fs/cache.go b/js/modules/k6/experimental/fs/cache.go index c7b9f41fc76..096ac8e8b7e 100644 --- a/js/modules/k6/experimental/fs/cache.go +++ b/js/modules/k6/experimental/fs/cache.go @@ -61,7 +61,7 @@ func (fr *cache) open(filename string, fromFs fsext.Fs) (data []byte, err error) // The underlying fsext.Fs.Open method will cache the file content during this // operation. Which will lead to effectively holding the content of the file in memory twice. - // However, as per #1079, we plan to eventually reduce our dependency on fsext, and + // However, as per #1079, we plan to eventually reduce our dependency on afero, and // expect this issue to be resolved at that point. // TODO: re-evaluate opening from the FS this once #1079 is resolved. f, err := fromFs.Open(filename) diff --git a/js/modules/k6/experimental/fs/file.go b/js/modules/k6/experimental/fs/file.go index cdebe2a6384..40b96174226 100644 --- a/js/modules/k6/experimental/fs/file.go +++ b/js/modules/k6/experimental/fs/file.go @@ -26,7 +26,7 @@ type file struct { // Stat returns a FileInfo describing the named file. func (f *file) stat() *FileInfo { filename := filepath.Base(f.path) - return &FileInfo{Name: filename, Size: len(f.data)} + return &FileInfo{Name: filename, Size: f.size()} } // FileInfo holds information about a file. diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index cf5964adc87..9e8af5c5ab4 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -109,7 +109,7 @@ func (mi *ModuleInstance) openImpl(path string) (*File, error) { fs, ok := initEnv.FileSystems["file"] if !ok { - common.Throw(mi.vu.Runtime(), errors.New("open() failed; reason: unable to access the file system")) + return nil, errors.New("open() failed; reason: unable to access the file system") } if exists, err := fsext.Exists(fs, path); err != nil { From 6ad5f9d53af981cc3c62dd6d8b48224a74e88b24 Mon Sep 17 00:00:00 2001 From: oleiade Date: Wed, 8 Nov 2023 10:06:11 +0100 Subject: [PATCH 08/18] Apply suggestions from code review --- js/modules/k6/experimental/fs/module_test.go | 53 ++++++++++---------- 1 file changed, 26 insertions(+), 27 deletions(-) diff --git a/js/modules/k6/experimental/fs/module_test.go b/js/modules/k6/experimental/fs/module_test.go index 30906e8ceda..7fc62420fbc 100644 --- a/js/modules/k6/experimental/fs/module_test.go +++ b/js/modules/k6/experimental/fs/module_test.go @@ -6,7 +6,6 @@ import ( "path/filepath" "testing" - "github.com/spf13/afero" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.k6.io/k6/js/compiler" @@ -55,8 +54,8 @@ func TestOpen(t *testing.T) { runtime, err := newConfiguredRuntime(t) require.NoError(t, err) - fs := newTestFs(t, func(fs afero.Fs) error { - fileErr := afero.WriteFile(fs, tt.wantPath, []byte("Bonjour, le monde"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + fileErr := fsext.WriteFile(fs, tt.wantPath, []byte("Bonjour, le monde"), 0o644) if fileErr != nil { return fileErr } @@ -156,7 +155,7 @@ func TestOpen(t *testing.T) { require.NoError(t, err) testDirPath := fsext.FilePathSeparator + "dir" - fs := newTestFs(t, func(fs afero.Fs) error { + fs := newTestFs(t, func(fs fsext.Fs) error { return fs.Mkdir(testDirPath, 0o644) }) @@ -207,8 +206,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("Bonjour, le monde"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("Bonjour, le monde"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -235,8 +234,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("01234"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("01234"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -254,7 +253,7 @@ func TestFile(t *testing.T) { } // We expect the buffer to be filled with the three first - // bytes of the file, namely '012'. + // bytes of the file. if (buffer[0] !== 48 || buffer[1] !== 49 || buffer[2] !== 50) { throw 'expected buffer to be [48, 49, 50], got ' + buffer + ' instead'; } @@ -267,7 +266,7 @@ func TestFile(t *testing.T) { } // We expect the buffer to hold the two last bytes of the - // file, namely '34', and as we read only two bytes, its last + // file, and as we read only two bytes, its last // one is expected to be untouched from the previous read. if (buffer[0] !== 51 || buffer[1] !== 52 || buffer[2] !== 50) { throw 'expected buffer to be [51, 52, 50], got ' + buffer + ' instead'; @@ -296,8 +295,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("012"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -333,8 +332,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("Bonjour, le monde"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("Bonjour, le monde"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -401,8 +400,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("012"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -436,8 +435,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("012"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -461,8 +460,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("012"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("012"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -488,8 +487,8 @@ func TestFile(t *testing.T) { require.NoError(t, err) testFilePath := fsext.FilePathSeparator + testFileName - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, testFilePath, []byte("hello"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("hello"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -608,7 +607,7 @@ func TestOpenImpl(t *testing.T) { runtime, err := newConfiguredRuntime(t) require.NoError(t, err) - fs := newTestFs(t, func(fs afero.Fs) error { + fs := newTestFs(t, func(fs fsext.Fs) error { return fs.Mkdir("/dir", 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs @@ -631,8 +630,8 @@ func TestOpenImpl(t *testing.T) { runtime, err := newConfiguredRuntime(t) require.NoError(t, err) - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, "/bonjour.txt", []byte("Bonjour, le monde"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, "/bonjour.txt", []byte("Bonjour, le monde"), 0o644) }) runtime.VU.InitEnvField.FileSystems["file"] = fs runtime.VU.InitEnvField.CWD = &url.URL{Scheme: "file", Path: "/dir"} @@ -674,10 +673,10 @@ func newConfiguredRuntime(t testing.TB) (*modulestest.Runtime, error) { // newTestFs is a helper function that creates a new in-memory file system and calls the provided // function with it. The provided function is expected to use the file system to create files and // directories. -func newTestFs(t *testing.T, fn func(fs afero.Fs) error) afero.Fs { +func newTestFs(t *testing.T, fn func(fs fsext.Fs) error) fsext.Fs { t.Helper() - fs := afero.NewMemMapFs() + fs := fsext.NewMemMapFs() err := fn(fs) if err != nil { From b73723bf6915d25890bbeff1bb8ae395a06f45b2 Mon Sep 17 00:00:00 2001 From: oleiade Date: Wed, 8 Nov 2023 10:08:42 +0100 Subject: [PATCH 09/18] Remove uses of afero in the experimental/fs module --- js/modules/k6/experimental/fs/cache_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/js/modules/k6/experimental/fs/cache_test.go b/js/modules/k6/experimental/fs/cache_test.go index b8dd7218ec6..b82d100f21c 100644 --- a/js/modules/k6/experimental/fs/cache_test.go +++ b/js/modules/k6/experimental/fs/cache_test.go @@ -3,8 +3,8 @@ package fs import ( "testing" - "github.com/spf13/afero" "github.com/stretchr/testify/assert" + "go.k6.io/k6/lib/fsext" ) func TestFileCacheOpen(t *testing.T) { @@ -14,8 +14,8 @@ func TestFileCacheOpen(t *testing.T) { t.Parallel() cache := &cache{} - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, "bonjour.txt", []byte("Bonjour, le monde"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, "bonjour.txt", []byte("Bonjour, le monde"), 0o644) }) _, gotBeforeOk := cache.openedFiles.Load("bonjour.txt") @@ -32,8 +32,8 @@ func TestFileCacheOpen(t *testing.T) { t.Parallel() cache := &cache{} - fs := newTestFs(t, func(fs afero.Fs) error { - return afero.WriteFile(fs, "bonjour.txt", []byte("Bonjour, le monde"), 0o644) + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, "bonjour.txt", []byte("Bonjour, le monde"), 0o644) }) firstData, firstErr := cache.open("bonjour.txt", fs) From 9f3d2d463b975ed999c3e34cbc26ab1f9bbe866d Mon Sep 17 00:00:00 2001 From: oleiade Date: Wed, 8 Nov 2023 10:22:12 +0100 Subject: [PATCH 10/18] Fix tests --- js/modules/k6/experimental/fs/file.go | 2 +- js/modules/k6/experimental/fs/module_test.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/js/modules/k6/experimental/fs/file.go b/js/modules/k6/experimental/fs/file.go index 40b96174226..2fcde11ce92 100644 --- a/js/modules/k6/experimental/fs/file.go +++ b/js/modules/k6/experimental/fs/file.go @@ -35,7 +35,7 @@ type FileInfo struct { Name string `json:"name"` // Size holds the size of the file in bytes. - Size int `json:"size"` + Size int64 `json:"size"` } // Read reads up to len(into) bytes into the provided byte slice. diff --git a/js/modules/k6/experimental/fs/module_test.go b/js/modules/k6/experimental/fs/module_test.go index 7fc62420fbc..5bb8b220247 100644 --- a/js/modules/k6/experimental/fs/module_test.go +++ b/js/modules/k6/experimental/fs/module_test.go @@ -577,10 +577,10 @@ func TestOpenImpl(t *testing.T) { cache: &cache{}, } - assert.Panics(t, func() { - //nolint:errcheck,gosec - mi.openImpl(testFileName) - }) + f, gotErr := mi.openImpl(testFileName) + + assert.Error(t, gotErr) + assert.Nil(t, f) }) t.Run("should return an error if the file does not exist", func(t *testing.T) { From 260307cf7846ffaec161a48643bba79309d624cb Mon Sep 17 00:00:00 2001 From: oleiade Date: Wed, 8 Nov 2023 10:24:57 +0100 Subject: [PATCH 11/18] Address linter issues --- js/modules/k6/experimental/fs/module.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index 9e8af5c5ab4..414376320c8 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -109,7 +109,7 @@ func (mi *ModuleInstance) openImpl(path string) (*File, error) { fs, ok := initEnv.FileSystems["file"] if !ok { - return nil, errors.New("open() failed; reason: unable to access the file system") + return nil, errors.New("open() failed; reason: unable to access the file system") } if exists, err := fsext.Exists(fs, path); err != nil { From 319cc34bbf7a6b7127f32e40c43e191ceb0fb8be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Th=C3=A9o=20Crevon?= Date: Fri, 10 Nov 2023 12:41:32 +0100 Subject: [PATCH 12/18] Update js/modules/k6/experimental/fs/cache.go Co-authored-by: Ivan <2103732+codebien@users.noreply.github.com> --- js/modules/k6/experimental/fs/cache.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/modules/k6/experimental/fs/cache.go b/js/modules/k6/experimental/fs/cache.go index 096ac8e8b7e..a25d2c02a82 100644 --- a/js/modules/k6/experimental/fs/cache.go +++ b/js/modules/k6/experimental/fs/cache.go @@ -30,7 +30,7 @@ type cache struct { // // The function cleans the provided filename using filepath.Clean before using it. // -// If the file was previously "opened" (and thus cached) by the registry, it +// If the file was previously "opened" (and thus cached), it // returns the cached content. Otherwise, it reads the file from the // filesystem, caches its content, and then returns it. // From 7a020d9198aaa46efbfb2bb0c7cc219743cc941c Mon Sep 17 00:00:00 2001 From: oleiade Date: Fri, 10 Nov 2023 13:07:04 +0100 Subject: [PATCH 13/18] Update `cache` type's documentation --- js/modules/k6/experimental/fs/cache.go | 34 +++++++++++++++++++++---- js/modules/k6/experimental/fs/module.go | 4 ++- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/js/modules/k6/experimental/fs/cache.go b/js/modules/k6/experimental/fs/cache.go index a25d2c02a82..e97c139126d 100644 --- a/js/modules/k6/experimental/fs/cache.go +++ b/js/modules/k6/experimental/fs/cache.go @@ -9,7 +9,35 @@ import ( "go.k6.io/k6/lib/fsext" ) -// cache is a cache of opened files. +// cache is a cache of opened files, designed to minimize redundant file reads, and +// avoid replicating the content of the files in memory as much as possible. +// +// Unlike the underlying [fsext.Fs] which also caches file contents, this cache minimizes +// synchronization overhead. [fsext.Fs], using `afero`, employs a [sync.RWMutex] for each +// file access, involving lock/unlock operations. Our cache, however, utilizes a concurrent-safe +// map (openedFiles), bypassing the need for these locks and enhancing performance. +// +// This cache could be seen as redundant, as the underlying [fsext.Fs] implementation +// already caches the content of the files it opens. However, the current implementation of +// [fsext.Fs] relies on `afero` under the hood, which in turn relies on a [sync.RWMutex] to +// protect access to the cached file content. This means that every time a file is opened, +// the `fsext.Fs` cache is accessed, and the [sync.RWMutex] is locked and unlocked. +// +// This cache is designed to avoid this synchronization overhead, by caching the content of +// the files in a map that is safe for concurrent use, and thus avoid the need for a lock. +// +// This leads to a performance improvement, at the cost of holding the content of the files +// in memory twice, once in the cache's `openedFiles` map, and once in the `fsext.Fs` cache. +// +// Note that the current implementation of the cache diverges from the guarantees expressed in the +// [design document] defining the `fs` module, as it we effectively hold the file's content in memory +// twice as opposed to once. +// +// Future updates (see [#1079](https://github.com/grafana/k6/issues/1079)) may phase out reliance on `afero`. +// Depending on our new choice for [fsext] implementation, this cache might become obsolete, allowing us +// to solely depend on [fsext.Fs.Open]. +// +// [#1079]: https://github.com/grafana/k6/issues/1079 type cache struct { // openedFiles holds a safe for concurrent use map, holding the content // of the files that were opened by the user. @@ -59,10 +87,6 @@ func (fr *cache) open(filename string, fromFs fsext.Fs) (data []byte, err error) return data, nil } - // The underlying fsext.Fs.Open method will cache the file content during this - // operation. Which will lead to effectively holding the content of the file in memory twice. - // However, as per #1079, we plan to eventually reduce our dependency on afero, and - // expect this issue to be resolved at that point. // TODO: re-evaluate opening from the FS this once #1079 is resolved. f, err := fromFs.Open(filename) if err != nil { diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index 414376320c8..9e7fb046a40 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -1,5 +1,7 @@ // Package fs provides a k6 module that allows users to interact with files from the -// local filesystem. +// local filesystem as per the [File API design document]. +// +// [File API design document]: https://github.com/grafana/k6/blob/master/docs/design/019-file-api.md#proposed-solution package fs import ( From 99cdc86367112e2e2419adcc3729fca5ac274c01 Mon Sep 17 00:00:00 2001 From: oleiade Date: Fri, 10 Nov 2023 13:11:51 +0100 Subject: [PATCH 14/18] Replace mentions of registry with cache --- js/modules/k6/experimental/fs/cache.go | 4 ++-- js/modules/k6/experimental/fs/module.go | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/js/modules/k6/experimental/fs/cache.go b/js/modules/k6/experimental/fs/cache.go index e97c139126d..b6432ff1468 100644 --- a/js/modules/k6/experimental/fs/cache.go +++ b/js/modules/k6/experimental/fs/cache.go @@ -54,7 +54,7 @@ type cache struct { } // open retrieves the content of a given file from the specified filesystem (fromFs) and -// stores it in the registry's internal `openedFiles` map. +// stores it in the cache's internal `openedFiles` map. // // The function cleans the provided filename using filepath.Clean before using it. // @@ -81,7 +81,7 @@ func (fr *cache) open(filename string, fromFs fsext.Fs) (data []byte, err error) if f, ok := fr.openedFiles.Load(filename); ok { data, ok = f.([]byte) if !ok { - panic(fmt.Errorf("registry's file %s is not stored as a byte slice", filename)) + panic(fmt.Errorf("cache's file %s is not stored as a byte slice", filename)) } return data, nil diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index 9e7fb046a40..cb29f1af9f2 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -140,8 +140,8 @@ func (mi *ModuleInstance) openImpl(path string) (*File, error) { path: path, data: data, }, - vu: mi.vu, - registry: mi.cache, + vu: mi.vu, + cache: mi.cache, }, nil } @@ -162,10 +162,10 @@ type File struct { // promises that are handled by the VU's runtime. vu modules.VU - // registry holds a pointer to the file registry this file is associated + // cache holds a pointer to the file cache this file is associated // with. That way we are able to close the file when it's not needed // anymore. - registry *cache + cache *cache } // Stat returns a promise that will resolve to a [FileInfo] instance describing From ac8a5a7deb2f9e70b1fbc00a6dc3f679bdbd9494 Mon Sep 17 00:00:00 2001 From: oleiade Date: Fri, 10 Nov 2023 13:19:53 +0100 Subject: [PATCH 15/18] Apply suggestions from code review --- js/modules/k6/experimental/fs/module.go | 36 ++++++++++++++----------- 1 file changed, 20 insertions(+), 16 deletions(-) diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index cb29f1af9f2..dd404adf0bd 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -259,25 +259,15 @@ func (f *File) Read(into goja.Value) *goja.Promise { func (f *File) Seek(offset goja.Value, whence goja.Value) *goja.Promise { promise, resolve, reject := f.vu.Runtime().NewPromise() - if common.IsNullish(offset) { - reject(newFsError(TypeError, "seek() failed; reason: the offset argument cannot be null or undefined")) - return promise - } - - var intOffset int64 - if err := f.vu.Runtime().ExportTo(offset, &intOffset); err != nil { - reject(newFsError(TypeError, "seek() failed; reason: the offset argument cannot be interpreted as integer")) - return promise - } - - if common.IsNullish(whence) { - reject(newFsError(TypeError, "seek() failed; reason: the whence argument cannot be null or undefined")) + intOffset, err := exportInt(f.vu.Runtime(), offset) + if err != nil { + reject(newFsError(TypeError, "seek() failed; reason: the offset argument "+err.Error())) return promise } - var intWhence int64 - if err := f.vu.Runtime().ExportTo(whence, intWhence); err != nil { - reject(newFsError(TypeError, "seek() failed; reason: the whence argument cannot be interpreted as integer")) + intWhence, err := exportInt(f.vu.Runtime(), whence) + if err != nil { + reject(newFsError(TypeError, "seek() failed; reason: the whence argument "+err.Error())) return promise } @@ -315,3 +305,17 @@ func isUint8Array(rt *goja.Runtime, o *goja.Object) bool { return true } + +//nolint:unparam +func exportInt(rt *goja.Runtime, v goja.Value) (int64, error) { + if common.IsNullish(v) { + return 0, errors.New("cannot be null or undefined") + } + + var i int64 + if err := rt.ExportTo(v, i); err != nil { + return 0, errors.New("cannot be interpreted as integer") + } + + return i, nil +} From d48fb673f8fa0bb4e4f8f36e8f1e2ae96373ab62 Mon Sep 17 00:00:00 2001 From: oleiade Date: Fri, 10 Nov 2023 14:53:27 +0100 Subject: [PATCH 16/18] Add seek tests covering happy case and fix SeekModeEnd starting offset --- js/modules/k6/experimental/fs/file.go | 2 +- js/modules/k6/experimental/fs/module.go | 3 +- js/modules/k6/experimental/fs/module_test.go | 35 ++++++++++++++++++++ 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/js/modules/k6/experimental/fs/file.go b/js/modules/k6/experimental/fs/file.go index 2fcde11ce92..b0e25416c35 100644 --- a/js/modules/k6/experimental/fs/file.go +++ b/js/modules/k6/experimental/fs/file.go @@ -99,7 +99,7 @@ func (f *file) Seek(offset int64, whence SeekMode) (int64, error) { return 0, newFsError(TypeError, "offset cannot be positive when using SeekModeEnd") } - newOffset = f.size() + offset + newOffset = (f.size() - 1) + offset default: return 0, newFsError(TypeError, "invalid seek mode") } diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index dd404adf0bd..9e731e2bf3f 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -306,14 +306,13 @@ func isUint8Array(rt *goja.Runtime, o *goja.Object) bool { return true } -//nolint:unparam func exportInt(rt *goja.Runtime, v goja.Value) (int64, error) { if common.IsNullish(v) { return 0, errors.New("cannot be null or undefined") } var i int64 - if err := rt.ExportTo(v, i); err != nil { + if err := rt.ExportTo(v, &i); err != nil { return 0, errors.New("cannot be interpreted as integer") } diff --git a/js/modules/k6/experimental/fs/module_test.go b/js/modules/k6/experimental/fs/module_test.go index 5bb8b220247..bba76a726ed 100644 --- a/js/modules/k6/experimental/fs/module_test.go +++ b/js/modules/k6/experimental/fs/module_test.go @@ -480,6 +480,41 @@ func TestFile(t *testing.T) { assert.NoError(t, err) }) + t.Run("seek", func(t *testing.T) { + t.Parallel() + + runtime, err := newConfiguredRuntime(t) + require.NoError(t, err) + + testFilePath := fsext.FilePathSeparator + testFileName + fs := newTestFs(t, func(fs fsext.Fs) error { + return fsext.WriteFile(fs, testFilePath, []byte("012"), 0o644) + }) + runtime.VU.InitEnvField.FileSystems["file"] = fs + + _, err = runtime.RunOnEventLoop(wrapInAsyncLambda(fmt.Sprintf(` + const file = await fs.open(%q) + + let newOffset = await file.seek(1, fs.SeekMode.Start) + + if (newOffset != 1) { + throw "file.seek(1, fs.SeekMode.Start) returned unexpected offset: " + newOffset; + } + + newOffset = await file.seek(-1, fs.SeekMode.Current) + if (newOffset != 0) { + throw "file.seek(-1, fs.SeekMode.Current) returned unexpected offset: " + newOffset; + } + + newOffset = await file.seek(0, fs.SeekMode.End) + if (newOffset != 2) { + throw "file.seek(0, fs.SeekMode.End) returned unexpected offset: " + newOffset; + } + `, testFilePath))) + + assert.NoError(t, err) + }) + t.Run("seek with invalid arguments should fail", func(t *testing.T) { t.Parallel() From 06d54a26991f6463671fad54a9981d86ec6ab4c7 Mon Sep 17 00:00:00 2001 From: oleiade Date: Fri, 10 Nov 2023 15:07:01 +0100 Subject: [PATCH 17/18] Address further seek related tests --- js/modules/k6/experimental/fs/file_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/js/modules/k6/experimental/fs/file_test.go b/js/modules/k6/experimental/fs/file_test.go index c93554d0ecf..58cfe80362b 100644 --- a/js/modules/k6/experimental/fs/file_test.go +++ b/js/modules/k6/experimental/fs/file_test.go @@ -210,21 +210,21 @@ func TestFileImpl(t *testing.T) { name: "seek using SeekModeEnd within file bounds should succeed", fileOffset: 20, args: args{-20, SeekModeEnd}, - wantOffset: 80, + wantOffset: 79, wantError: false, }, { name: "seek using SeekModeEnd beyond file start should fail", fileOffset: 20, args: args{-110, SeekModeEnd}, - wantOffset: 20, + wantOffset: 19, wantError: true, // File is 100 bytes long }, { name: "seek using SeekModeEnd and a positive offset should fail", fileOffset: 20, args: args{10, SeekModeEnd}, - wantOffset: 20, + wantOffset: 19, wantError: true, }, { From 01f9c9401c49319ec969aa3bd62265c43310a974 Mon Sep 17 00:00:00 2001 From: oleiade Date: Fri, 10 Nov 2023 15:07:14 +0100 Subject: [PATCH 18/18] Fix exportInt function --- js/modules/k6/experimental/fs/module.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/js/modules/k6/experimental/fs/module.go b/js/modules/k6/experimental/fs/module.go index 9e731e2bf3f..bfe5e861da9 100644 --- a/js/modules/k6/experimental/fs/module.go +++ b/js/modules/k6/experimental/fs/module.go @@ -7,6 +7,7 @@ package fs import ( "errors" "fmt" + "reflect" "github.com/dop251/goja" "go.k6.io/k6/js/common" @@ -259,13 +260,13 @@ func (f *File) Read(into goja.Value) *goja.Promise { func (f *File) Seek(offset goja.Value, whence goja.Value) *goja.Promise { promise, resolve, reject := f.vu.Runtime().NewPromise() - intOffset, err := exportInt(f.vu.Runtime(), offset) + intOffset, err := exportInt(offset) if err != nil { reject(newFsError(TypeError, "seek() failed; reason: the offset argument "+err.Error())) return promise } - intWhence, err := exportInt(f.vu.Runtime(), whence) + intWhence, err := exportInt(whence) if err != nil { reject(newFsError(TypeError, "seek() failed; reason: the whence argument "+err.Error())) return promise @@ -306,15 +307,18 @@ func isUint8Array(rt *goja.Runtime, o *goja.Object) bool { return true } -func exportInt(rt *goja.Runtime, v goja.Value) (int64, error) { +func exportInt(v goja.Value) (int64, error) { if common.IsNullish(v) { return 0, errors.New("cannot be null or undefined") } - var i int64 - if err := rt.ExportTo(v, &i); err != nil { - return 0, errors.New("cannot be interpreted as integer") + // We initially tried using `ExportTo` with a int64 value argument, however + // this led to a string passed as argument not being an error. + // Thus, we explicitly check that the value is a number, by comparing + // its export type to the type of an int64. + if v.ExportType().Kind() != reflect.Int64 { + return 0, errors.New("must be a number") } - return i, nil + return v.ToInteger(), nil }