Fix fdRead: handle io.Reader corner cases

[ncruces]

These are to handle corner cases described in io.Reader.

For a partial read readv really shouldn't continue reading into the following buffer.

And according to io.Reader, callers should handle any read bytes before considering the error.

[codefromthecrypt]

Thanks for the help. I would like to progress this with you.

The one case is very curious why we are ignoring the error when n != 0. I'd like to know what that error was when you can safely do this.

For example, it makes sense to ignore error when io.EOF, but why would we ignore any type of error in this case? can you give more info?

At the moment Test_fdWrite isn't a matrix, but if we don't test this it will likely become re-broken. Can you attempt to matrix this to include your corner cases?

[codefromthecrypt]

doesn't this swallow the error? how can they consider the err if there's none returned?

[ncruces]

TLDR: they handle the error on their next call to readv (fread, etc).

If you assume the error is stable (e.g. an external disk was removed), once they readv again they'll get the same error.

OTOH, if the error is transient (which is unlikely for real filesystems), once they call readv again the error will be lost, but at least there is no data loss.

So yes, this may swallow errors in 2 situations:

1. if n bytes are enough for the caller, and they don't readv again.
2. if err is transient, and isn't returned on a future call to readv.

Both seem legitimate reasons to ignore the error, rather than loosing/ignoring the data.

[codefromthecrypt]

cool

[ncruces]

Yes, this was the one that worried me the most.

[ncruces]

I can look into tests but it might take me a while, as I'll be OOO for a couple of weeks.

#739 was the issue that was blocking me, this was just something I found while debugging why reads were failing me, but it turned out not to be the issue.

Here, the n < len(b) case seems simple to solve. I can split it off to another PR if you'd prefer (or keep just that one and ignore the other).

The other one (returning data and an error) probably doesn't happen much in the real world to begin with.

[codefromthecrypt]

@ncruces thanks for the background.

I think some libraries will do their own sort of error mediation above the syscall layer wrt some read failure, though I understand that the read already occurred and a stream often can't be re-read. Mainly, it feels a little dodgy. If we do something that is a bit dodgy, what I would say is to cite something else (ex x/sys for some arch or some underlying impl). Meanwhile, we can add a comment about the possible issue and move on for now.

Meanwhile, I'll help with the tests on this PR since you'll be out. I'll also look for any other place where we have a similar read loop. Mainly, I appreciate your attention, focus and research which is worth more to the project than having your personally do the test matrix stuff, especially as we plan to do our first beta in a couple weeks.

TL;DR; is I'll push a commit to this branch and merge while you're gone. Have a nice one!

[ncruces]

What you could also do, since you can't return both the error and the data, is to do as bufio.Reader and store the error condition to return it (and clear it) on the next call.

[codefromthecrypt]

well, we can write the data before returning the errno. that's fine

[codefromthecrypt]

we want to avoid too much logic and allocations at the syscall abstraction which is repeated at higher abstractions, so trying to do compensating buffering which may also occur in compiled wasm probably isn't the right choice, iotw

[ncruces]

well, we can write the data before returning the errno. that's fine

Yes, but callers of readv won't inspect it if you return an error, so the data is lost forever. That's the contract.

[codefromthecrypt]

Yes, but callers of readv won't inspect it if you return an error, so the data is lost forever. ...

sounds fine as I wasn't particularly interested in writing it on error anyway ;)

[codefromthecrypt]

@ncruces I didn't quite read your comment below correctly first time. My gut feel is storing extra state about an error might be problematic, but then again WebAssembly doesn't really implement concurrency, anyway, so it might not be so bad. Will think and decide tomorrow after playing around

What you could also do, since you can't return both the error and the data, is to do as bufio.Reader and store the error condition to return it (and clear it) on the next call.

[codefromthecrypt]

backfilled docs, will do the test tomorrow

[ncruces]

I just read your rational, the explanation is perfect. You can ignore the rest of my comment, since I'm just restating your rational, basically.

What I meant was storing the read error in FileEntry, maybe just the pending Errno, then returning it from FdReader.
But I don't immediately see how readv can set this, there'd need to be another function in internal/sys/fs.go to set it.
Not a very satisfactory solution.

Since WASI is somewhat in flux, I think it's reasonable to either swallow the error, or punt this particular issue entirely.
This mismatch in APIs is not likely to cause issues in actual use.

The most important fix here is Partial read, don't read into the next buffer.

PS: if I have a lot of spare time I may have a go at running (some form of) SQLite inside WASI. That should be a nice stress test for the existing and missing WASI APIs. And it should be fun to (somehow) compare SQLite on top of wazero vs. modernc.org/sqlite or github.com/cvilsmeier/sqinn-go.

[codefromthecrypt]

@mathetake PTAL!

[codefromthecrypt]

thanks again @ncruces and looking forward to your future developments