fix(p3-shim): report invalid read offsets through future

[andreiltd]

This is attempt to fix #1787

I think to resolve #1787 we will need a fix on the wasi-testsuite side as well. The test is checking file size after each stream.write  completed and I think the check should be deferred to until after the stream future resolves.

Specifically, I think this check is racy: https://github.com/WebAssembly/wasi-testsuite/blob/main/tests/rust/wasm32-wasip3/src/bin/filesystem-io.rs#L115

Edit: see WebAssembly/wasi-testsuite#278

[vados-cosmonic]

Hey @andreiltd thanks for the PR, this looks great -- I took a look at this too, and I ran into a couple other things that I thought were the issue instead:

1. The handleRead bug I solved by not forwarding the error on the stream, but closing it instead in the catch (i.e. not writer.abort(err), but writer.close() and rethrow). In the case of read-via-stream the stream actually doesn't have a method of returning an error, so would be keen to hear what cases I'm missing there.

Any reason we shouldn't do that? I'm not sure what case the stream aborting the errors was created for.

Moving the check out certainly works as well -- happy to take that as a fix, but would like to know what the case is that we do want to abort the stream with the error.

2. In my local testing, the ordering of the operations is actually fine/right in the prepend testing -- the problem is that the events happen in a consistenty bad order event-loop wise:

• the prepend starts
• the initial stat is performed
• the write starts (but writeViaStream suspends)
• the during-write stat is performed before the writeViaStream completes, but since the write actually depends on the stat completing (before the write end is dropped), we're stuck.

There are two issues:

• Operations on the same FD can be misordered due to event loop vagaries, despite coming in with the right timing
• A "write" is currently modeled as one operation by the worker, but they could be a resumable operation, depending on if tasks get blocked on something like a read.

Right now what I've been prototyping locally looks like the following:

  async writeViaStream(data, offset) {
    this.#ensureWritable(data);
    const stream = readableByteStreamFromReader(data, { name: "file write data" });

    try {
      let release = await this.lockFDForOperation({ fd: this.#handle.fd, op: `write on fd [${this.#handle.fd}]` });

      const work = worker().run({ op: "write", fd: this.#handle.fd, offset, stream }, [stream]);
      while (true) {
          const finished = await Promise.race([
              new Promise((resolve) => setTimeout(() => resolve(null), 100)),
              work,
          ]);

          // if we got the worker finished, break out
          if (finished !== null) {
              break;
          }

          // If we didn't, release and re-acquire the FD operation
          release()
          release = await this.lockFDForOperation({ fd: this.#handle.fd, op: `write on fd [${this.#handle.fd}]` });
      }

      release();
    } catch (err) {
      throw FSError.from(err);
    }
  }

(stat() uses similar machinery, but just the lock bit)

This prototype isn't ideal since it's just timeout based, but being able to treat worker operations as a generator (knowing when it has made some progress, or has yielded because it couldn't make any progress) and run them until completion (with some time to allow other operations to take place) along with some queueing should solve this problem in a much more satisfying way.

Basically the idea is that we need a way to know that the write operation made some progress, and to allow the waiting stat() operation to make some progress. In this case specifically the partial write would succeed, the write would return, and then the stat would see the additionally written data.

While we can change the upstream tests, I think the code as written is not unreasonable so it might be a good idea to try to investigate avenues for supporting it, wasmtime does.

[andreiltd]

I'm not sure what case the stream aborting the errors was created for.

Hey, so my mental model for the difference between closing and aborting stream is:

• we want to close stream normally when the error is in filesystem domain and use the future to surface the error and,
• we want to abort the stream if error happens in transport itself, e.g. serialization, cancelled consumer etc.

but it looks like wasi wants both to happen: the stream closes with an  error-context, and the future resolves to err(error-code) . close() will loose the context so it looks like maybe abort is better here?

https://github.com/WebAssembly/WASI/blob/main/proposals/filesystem/wit/types.wit#L321

Operations on the same FD can be misordered due to event loop vagaries

Right, that seems like an issue, but maybe there is some lock free solution? For example creating a queue of operations on the worker side?

While we can change the upstream tests, I think the code as written is not unreasonable so it might be a good idea to try to investigate avenues for supporting it, wasmtime does.

I think the fix in wasi-testsuite is still correct. I mean wasmtime can finish writing to the file before we observe stream completed, but the real confirmation should be future, no?

[vados-cosmonic]

I'm not sure what case the stream aborting the errors was created for.

Hey, so my mental model for the difference between closing and aborting stream is:

* we want to close stream normally when the error is in filesystem domain and use the future to surface the error and,

* we want to abort the stream if error happens in transport itself, e.g. serialization, cancelled consumer etc.

but it looks like wasi wants both to happen: the stream closes with an error-context, and the future resolves to err(error-code) . close() will loose the context so it looks like maybe abort is better here?

https://github.com/WebAssembly/WASI/blob/main/proposals/filesystem/wit/types.wit#L321

Yeah, so in this case the error is in the filesystem domain, so we'd theoretically want to throw the error here too, no? I think this might be pointing us to needing to envelope, or at least catch all errors on the bindgen side and return them as enveloped values.

Operations on the same FD can be misordered due to event loop vagaries

Right, that seems like an issue, but maybe there is some lock free solution? For example creating a queue of operations on the worker side?

Yeah whether the queue is on the invoking side or the worker side is fine with me, IFF you actually get to the worker side in time to queue properly. The problem is that worker().run(...) happens on the invoking side and then control goes to stat() which doesn't hit the worker at all. There are multiple ways to go about it, but you may have to move stat into the worker, or do the coordination on the invocation side.

That said we need both queueing and resumable operations.

While we can change the upstream tests, I think the code as written is not unreasonable so it might be a good idea to try to investigate avenues for supporting it, wasmtime does.

I think the fix in wasi-testsuite is still correct. I mean wasmtime can finish writing to the file before we observe stream completed, but the real confirmation should be future, no?

Yeah I'm not saying changing the upstream code is wrong, it's certainly less racy to do it that way -- I'm saying that the code that worked should work, and exposes a weakness in the current implementation.

I think wasmtime does a write-wait-yield here -- there are writes on both sides of the stat. So the first write finishes, but then blocks waiting for more to write. Jco code does the same -- but the problem is that we have no way to come back out of the write after the partial write succeeds to allow something else to run so we get stuck.

The write task gets stuck waiting for stream read to supply it more data forever, and never gives other tasks a chance. Modeled as a queue, you'd have the same problem, just as the first task in the queue never yielding after making a little bit of progress. If we had a way to do some work then make progress, we could accept this kind of program.

[vados-cosmonic]

OK, so thinking about this some more, I think this boils down to a few things:

1. We should be able to do things from the invoker side when the worker is blocked
2. We probably don't need explicit queuing and probably shouldn't add it, because of (3) below
3. The racy test should be updated (but we should keep the racy version in Jco as a regression test)
4. The wasi:filesystem API should likely be updated to return a stream of acks for partial writes, rather than an all-at-once future (and the test is using stream value delivery as write operation completion, which is true in wasmtime but not here).

While I wanted to solve this by ordering the operations, I think adding that norm to wasi:filesystem is probably a bad idea, if we can fix it with a better write-via-stream API.