Add support for FileSystemSyncAccessHandle in OPFS

[ansemjo]

This could partly fix #32 by allowing to use FileSystemSyncAccessHandles in Workers for single files opened from OPFS as noted in my last comment. Usage example:

// open synchronous handle to a file in opfs
const opfs = await navigator.storage.getDirectory();
const file = await opfs.getFileHandle("example.txt");
const handle = await file.createSyncAccessHandle();

// populate the virtual filesystem for shim
const fds: Array<Fd> = [
  new OpenFile(new File([])), // stdin
  new OpenFile(new File([])), // stdout
  new OpenFile(new File([])), // stderr
  new PreopenDirectory("/", {
    "example.txt": new OPFSFile(handle),
  }),
];
const shim = new WASI(args, envs, fds);

This should currently be considered very WIP, as I've developed this out-of-tree in my project and then tried to copy it into this fork. That means I haven't actually run the code here yet. In my project I could successfully read a small file though:

[ansemjo]

• One problem that I already noticed, is that the file is never actually closed though. My experiment in Rust just uses std::fs::read() to read the contents and so apparently does not explicitly call fd_close() anywhere. The result is that trying to access the file a second time fails with a NoModificationAllowedError: No modification allowed.
• Also, I should probably implement more of the Fd methods ... fd_sync() comes to mind, as the FileSystemSyncAccessHandle brings a flush() method. We should probably use that.

[bjorn3]

Thanks a lot for working on this!

One problem that I already noticed, is that the file is never actually closed though. My experiment in Rust just uses std::fs::read() to read the contents and so apparently does not explicitly call fd_close() anywhere. The result is that trying to access the file a second time fails with a NoModificationAllowedError: No modification allowed.

Weird that it is never closed. Can you use the strace wrapper to show all syscalls that are made? In any case there should be code added to handle opening the same file multiple times. At least once directories are supported.

[bjorn3]

Maybe these cases should all be replaced with an open() method on a new DirEntry or Inode base class which File, SyncOPFSFile and Directory would then derive from?

[ansemjo]

Don't they all already derive from Fd? We could just use duck-typing to check if the entry has an open() property and call that.

[ansemjo]

Well, none of File | SyncOPFSFile | Directory derived from Fd but Typescript handles the type union to check if .open() exists just fine without a common base class: 512a46d

[ansemjo]

Thanks a lot for working on this!

One problem that I already noticed, is that the file is never actually closed though. My experiment in Rust just uses std::fs::read() to read the contents and so apparently does not explicitly call fd_close() anywhere. The result is that trying to access the file a second time fails with a NoModificationAllowedError: No modification allowed.

Weird that it is never closed. Can you use the strace wrapper to show all syscalls that are made? In any case there should be code added to handle opening the same file multiple times. At least once directories are supported.

Maybe I should clarify: by opening a second time I mean starting a second WebAssembly instance, which also tries to open the file from OPFS. So technically, the previous instance simply still holds the lock on the file. My expectation would have been that at the end of wasi.start(instance) all opened files would be closed again but I am not sure if that'd be correct behaviour.

Anyway, how do I use the strace wrapper? Just wrap the "wasi_snapshot_preview1" import as seen in the example?

let inst = await WebAssembly.instantiate(wasm, {
  "wasi_snapshot_preview1": strace(w.wasiImport, ["fd_prestat_get"]),
});

[bjorn3]

Maybe I should clarify: by opening a second time I mean starting a second WebAssembly instance, which also tries to open the file from OPFS. So technically, the previous instance simply still holds the lock on the file. My expectation would have been that at the end of wasi.start(instance) all opened files would be closed again but I am not sure if that'd be correct behaviour.

For reactor style wasi modules as handled by .initialize() fds should be kept. For command style wasi modules as handled by .start() fds could be closed at the end. That would be a breaking change though. Maybe you could add a FIXME to start() to do this in v0.3?

Anyway, how do I use the strace wrapper? Just wrap the "wasi_snapshot_preview1" import as seen in the example?

Yes:

browser_wasi_shim/examples/rustc.html

Lines 129 to 131 in ee4f287

	let inst = await WebAssembly.instantiate(wasm, {
	"wasi_snapshot_preview1": strace(w.wasiImport, ["fd_prestat_get"]),
	});

[ansemjo]

Aha! Mea culpa .. the Rust experiment actually does close the file as seen here:

This is the code that is executed there:

fn main() {
  println!("This is Rust.");

  // print commandline arguments
  let args: Vec<String> = std::env::args().collect();
  let name = args[0].clone();
  println!("file '{name}' was called with arguments: {args:?}");

  // print environment variables
  println!("environment variables:");
  for (key, value) in std::env::vars() {
    println!(" - {}: {}", key, value);
  };

  // list root filesystem
  match std::fs::read_dir("/") {
    Err(e) => println!("ERR: couldn't open directory '/': {e}"),
    Ok(iter) => {
      println!("listing '/' contents:");
      for item in iter {
        println!(" - {}", item.unwrap().path().display());
      }
    }
  };

  // try to read a specific file
  const FILENAME: &str = "/hello.txt";
  match std::fs::read(FILENAME) {
    Err(e) => println!("ERR: couldn't open file '{FILENAME}': {e}"),
    Ok(content) => {
      let text = String::from_utf8_lossy(&content);
      println!("'{FILENAME}': {text}");
    },
  }

}

My TinyGo experiment however, does not ..

package main

import (
	"fmt"
	"io"
	"os"
)

func main() {
	fmt.Println("This is TinyGo.")

	// print commandline arguments
	args := os.Args
	name := args[0]
	fmt.Printf("file '%s' was called with arguments: %v\n", name, args)

	// print environment variables
	fmt.Println("environment variables:")
	for _, env := range os.Environ() {
		fmt.Printf(" - %s\n", env)
	}

	// list root filesystem
	contents, err := os.ReadDir("/")
	if err != nil {
		fmt.Printf("ERR: couldn't open directory '/': %s\n", err)
	} else {
		fmt.Println("listing '/' contents:")
		for _, item := range contents {
			fmt.Printf(" - %s\n", item.Name())
		}
	}

	// try to read a specific file
	const FILENAME = "/hello.txt"
	file, err := os.Open(FILENAME)
	if err != nil {
		fmt.Printf("ERR: couldn't open file '%s': %s\n", FILENAME, err)
	} else {
		bytes, err := io.ReadAll(file)
		if err != nil {
			fmt.Printf("ERR: couldn't read file '%s': %s\n", FILENAME, err)
		} else {
			fmt.Printf("'%s': %s\n", FILENAME, string(bytes))
		}
	}

}

I mean, yeah .. I neither call nor defer file.Close() anywhere. I've added a FIXME in the last commit.

[ansemjo]

It is definitely incorrect to close the handle in fd_close() though, as that makes the file unusable for the rest of the execution. So this should also be handled in SyncOPFSFile after WASI.start().

[ansemjo]

I've tested writes and caught an OOB bug in the process; added support for FDFLAGS_APPEND; and removed my console.log lines.

Regarding the closing of the FileSystemSyncAccessHandle: I'm thinking that maybe we just shouldn't bother with it, since the way I have implemented it here, I already accept a handle. Which means it was created externally and maybe we shouldn't make assumptions and close it.

The Inode base class idea I'd leave for another pull request. 😛

If you don't have any further comments, I think you might merge this now as the basic read / write / seek / append operations work now, as far as I can tell.

[bjorn3]

I'm going to test this myself tomorrow. If I don't see anything weird I will merge it.

[ansemjo]

@bjorn3 Now that I've added fd_flags to entry.open() below, maybe the truncate call should also be moved to the File | SyncOPFSFile constructors?

[bjorn3]

Got it working locally.

[bjorn3]

Do you have more changes you want or shall I publish a new release with this?

[ansemjo]

Got it working locally.

Great! 😊

Do you have more changes you want or shall I publish a new release with this?

Right now I'm just glad you found this useful enough to merge so quickly. :) I see you reopened the issue to track work on the async API / whole directories .. that'd be great but I don't think I can justify the effort to even attempt that right now.

It probably should've been part of this PR but maybe add another usage example on how to use this new SyncOPFSFile, since the only way to find it right now is to look through the code or find this issue. Is your test code suitable for this?

Other than that I don't have anything that should be a blocker for a new release, I think.

[bjorn3]

I see you reopened the issue to track work on the async API / whole directories .. that'd be great but I don't think I can justify the effort to even attempt that right now.

No problem.

It probably should've been part of this PR but maybe add another usage example on how to use this new SyncOPFSFile, since the only way to find it right now is to look through the code or find this issue. Is your test code suitable for this?

The code I used to test it was using the OPFS file as stderr for rustc.wasm and then manually verifying that it wrote to the file in the browser console. Not really something usable as test for others.