Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge sock_send/sock_recv with fd_write/fd_read #4

Closed
sunfishcode opened this issue Apr 3, 2019 · 11 comments
Closed

Merge sock_send/sock_recv with fd_write/fd_read #4

sunfishcode opened this issue Apr 3, 2019 · 11 comments
Labels
proposal A discussion about a specific actionable item wasi-filesystem Issues targeted for a `wasi_unstable_filesystem` module wasi-io Issues targeted for a `wasi_unstable_io` module wasi-network-sockets Issues targeted for a possible `wasi_unstable_sockets` module

Comments

@sunfishcode
Copy link
Member

WASI currently has two pairs of functions which are similar to each other: sock_send/sock_recv and fd_write/fd_read. This PR describes a plan for merging them, in favor of fd_write/fd_read.

Background

The reason why send and recv are separate in POSIX is that they add flags arguments. POSIX says that send and recv are equivalent to write and read when no flags are set.

WASI's sock_send doesn't currently support any flags. WASI's sock_recv supports __WASI_SOCK_RECV_PEEK and __WASI_SOCK_RECV_WAITALL which correspond to MSG_PEEK and MSG_WAITALL in POSIX. Both of these operations conceptually could work on files, however typical operating systems only support them on sockets.

On Linux, there is a subtle difference between recv and read: "If a zero-length datagram is pending, read(2) and recv() with a flags argument of zero provide different behavior. In this circumstance, read(2) has no effect (the datagram remains pending), while recv() consumes the pending datagram." It is possible that applications could depend on this subtle difference, but the only reference to it I've been able to find is the git commit which added this line to the man page, which describes a bug where "[...] we would end up in a busy loop when we were using read(2). Changing to recv(2) fixed the issue [...]". The recv behavior, is what the code in that bug wanted, and is the more intuitive behavior.

The cause of this subtlety is that read special-cases a 0 return value to mean the end-of-file/stream has been reached. That creates an ambiguity when reading a zero-length datagram.

Proposal

  • Remove sock_send and sock_recv.
  • Add __wasi_siflags_t and __wasi_riflags_t arguments to fd_write and fd_read, respectively.
  • Make fd_read return __WASI_EMSGSIZE when receiving a datagram which is larger than the provided buffer. And remove __WASI_SOCK_RECV_DATA_TRUNCATED, which is what sock_recv used in that case. WASI libc will check for this and to continue to implement the POSIX API (MSG_TRUNC).
  • Make fd_read return __WASI_EEOS, a new errno code, when the end-of-file/stream is reached. This eliminates the ambiguity of the special case for 0. WASI libc will check for this and continue to implement the POSIX API with 0 being a special case.
  • Add rights for __WASI_RIGHT_FD_READ_PEEK and __WASI_RIGHT_FD_READ_WAITALL, which are required to use the __WASI_SOCK_RECV_PEEK and __WASI_SOCK_RECV_WAITALL flags, respectively. These rights would not be granted for file-based file descriptors on OS's that don't support these features on files.
  • Remove the fs_filetype field from the fdstat_t struct. This further hides unnecessary differences between sockets and files. fd_fdstat_get is an otherwise ambient authority, meaning anyone can do it on any open file descriptor. The file type is still accessible, via fd_filestat_get, but that requires (__WASI_RIGHT_FD_FILESTAT_GET).
  • That happens to leave us with no easy way to implement isatty, so add a __WASI_RIGHT_FD_ISATTY right, to indicate whether a file descriptor is known to be a terminal. This is a little unusual as it's not a typical right, as it's not associated with an operation. However, this right makes it simple to implement isatty, which is used by libc to do line buffering for stdout when it's on a tty.

And some minor tidying:

  • Rename sock_shutdown to fd_shutdown, and make it a file descriptor operation that happens to depend on the __WASI_RIGHT_SOCK_SHUTDOWN right, which on typical implementations will only get granted for sockets. This is the last remaining sock_* function.
  • Rename __WASI_RIGHT_SOCK_SHUTDOWN to __WASI_RIGHT_FD_SHUTDOWN.
  • Rename __WASI_SOCK_RECV_PEEK and __WASI_SOCK_RECV_WAITALL to say FD_READ instead of SOCK_RECV.

Miscellaneous notes

The change to make fd_read return __WASI_EEOS on end-of-file/stream also fixes an oddity in POSIX in which many applications do an extra read call after the EOF is encountered, in order to get a 0 return from read to confirm they've actually reached the end. That said, implementations on POSIX hosts won't be able to report __WASI_EEOS until they get a 0 from read themselves, so in practice there will still be an extra read on such systems.

@alexcrichton
Copy link
Contributor

This all sounds like a great idea to me, funneling these through the same syscall seems pretty natural fits well with the existing capabilities and such. I'm curious about a few parts though:

That happens to leave us with no easy way to implement isatty

I was initially wondering how this affects isatty as it has to do with sockets/files, but reading up on the implementation it looks like __wasi_fd_fdstat_get is used to see whether it's a character device you can't seek/tell on. Would it make sense to get fd_filestat_get working with character devices or should that only work with literal files on the filesystem?

Otherwise, is this something that we'd have to add more rights for in the future? In that would new kinds of file descriptors get their own rights? (just wondering if this is a "slippery slope" or pretty contained)

The change to make fd_read return __WASI_EEOS on end-of-file/stream also fixes an oddity in POSIX in which many applications do an extra read call after the EOF is encountered

I was curious about this and wondering how we'd manage that. Is it the intention that __WASI_EEOS would be returned on a successful read instead of __WASI_ESUCCESS? For example if I read a 15 byte file with a 30 byte buffer, would I read 15 bytes and get a __WASI_EEOS return code?

@sunfishcode
Copy link
Member Author

I was initially wondering how this affects isatty as it has to do with sockets/files, but reading up on the implementation it looks like __wasi_fd_fdstat_get is used to see whether it's a character device you can't seek/tell on. Would it make sense to get fd_filestat_get working with character devices or should that only work with literal files on the filesystem?

Otherwise, is this something that we'd have to add more rights for in the future? In that would new kinds of file descriptors get their own rights? (just wondering if this is a "slippery slope" or pretty contained)

My theory is that this isn't a slippery slope because isatty is special by virtue of being one of the things implicitly called by libc for stdout support in C (and similar in several other languages). fd_filestat_get does work on character devices, but I was trying to avoid importing it, since this affects hello world in these languages. However, you may be right that that's the better solution here, rather than the special __WASI_RIGHT_FD_ISATTY right.

Is it the intention that __WASI_EEOS would be returned on a successful read instead of __WASI_ESUCCESS? For example if I read a 15 byte file with a 30 byte buffer, would I read 15 bytes and get a __WASI_EEOS return code?

Yes, that's right. Another option is to add a bool output parameter to fd_read to indicate end-of-file/stream explicitly. My rough sense is that an output parameter is partially redundant because it'll never be true when the return value isn't __WASI_ESUCCESS, however either way would work.

@alexcrichton
Copy link
Contributor

Ah ok, if fd_filestat_get works then I might personally advocate for that rather than adding a new right, but of course leaving the option open to adding a new right later if it becomes necessary.

For __WASI_EEOS sounds good to me! I'd personally consider that the ergonomics of these APIs (e.g. how usable they are to call directly) isn't too important as they'll almost always want to be wrapped with something else. In that sense it's easy enough for libraries deep in the stack to check for EEOS or ESUCCESS and everyone else wouldn't have to worry about it.

@npmccallum
Copy link
Contributor

  • Add rights for __WASI_RIGHT_FD_READ_PEEK and __WASI_RIGHT_FD_READ_WAITALL, which are required to use the __WASI_SOCK_RECV_PEEK and __WASI_SOCK_RECV_WAITALL flags, respectively. These rights would not be granted for file-based file descriptors on OS's that don't support these features on files.
  • That happens to leave us with no easy way to implement isatty, so add a __WASI_RIGHT_FD_ISATTY right, to indicate whether a file descriptor is known to be a terminal. This is a little unusual as it's not a typical right, as it's not associated with an operation. However, this right makes it simple to implement isatty, which is used by libc to do line buffering for stdout when it's on a tty.

When you use these rights for double duty, how does the caller determine the difference between, expressed in POSIX errno terms, ENOPERM and ENOTSOCK?

I would expect that using the flags on read() and write() would tell me that the flags don't apply to my type of file descriptor not that I'm missing some right.

@npmccallum
Copy link
Contributor

Is it the intention that __WASI_EEOS would be returned on a successful read instead of __WASI_ESUCCESS? For example if I read a 15 byte file with a 30 byte buffer, would I read 15 bytes and get a __WASI_EEOS return code?

Yes, that's right. Another option is to add a bool output parameter to fd_read to indicate end-of-file/stream explicitly. My rough sense is that an output parameter is partially redundant because it'll never be true when the return value isn't __WASI_ESUCCESS, however either way would work.

I haven't actually looked at the error handling part of WASI yet. However, I presume the libc implementation would return 15 in this case and expect a second read() call. On the second read() call we would return 0. Is this correct?

@taralx
Copy link

taralx commented Apr 15, 2019

For what it's worth, we should encourage implementations to support the full set of operations even on files - no reason you couldn't support peek on seekable data, for example.

@sunfishcode
Copy link
Member Author

When you use these rights for double duty, how does the caller determine the difference between, expressed in POSIX errno terms, ENOPERM and ENOTSOCK?

One option is to rewrite the errno value to ENOTSOCK in libc; that's what CloudABI does to solve some related problems, for example.

I would expect that using the flags on read() and write() would tell me that the flags don't apply to my type of file descriptor not that I'm missing some right.

POSIX read() and write() work on both files and sockets. POSIX defines recv() and send() with no flags to be equivalent to read() and write(), except that they fail with ENOTSOCK if used on files. This restriction is arbitrary; is it useful and worth keeping?

I haven't actually looked at the error handling part of WASI yet. However, I presume the libc implementation would return 15 in this case and expect a second read() call. On the second read() call we would return 0. Is this correct?

Yes. You can always keep calling read after you reach EOF, and it'll return 0 (as long as the file isn't modified), so programs that expect to read until they get a 0 will work as intended.

@npmccallum
Copy link
Contributor

When you use these rights for double duty, how does the caller determine the difference between, expressed in POSIX errno terms, ENOPERM and ENOTSOCK?

One option is to rewrite the errno value to ENOTSOCK in libc; that's what CloudABI does to solve some related problems, for example.

It sucks to consume a (likely) second context switch on ENOTCAPABLE. But hopefully that isn't common. Hopefully it is also not in a critical code path. I suspect that is not likely.

I would expect that using the flags on read() and write() would tell me that the flags don't apply to my type of file descriptor not that I'm missing some right.

POSIX read() and write() work on both files and sockets. POSIX defines recv() and send() with no flags to be equivalent to read() and write(), except that they fail with ENOTSOCK if used on files. This restriction is arbitrary; is it useful and worth keeping?

I think that is exactly my question. That behavior is very well known. I definitely imagine there are libraries that are using that behavior instead of a more explicit test. However, I don't have a good sense of the actual cost in choosing not to preserve that behavior.

I haven't actually looked at the error handling part of WASI yet. However, I presume the libc implementation would return 15 in this case and expect a second read() call. On the second read() call we would return 0. Is this correct?

Yes. You can always keep calling read after you reach EOF, and it'll return 0 (as long as the file isn't modified), so programs that expect to read until they get a 0 will work as intended.

+1

@taralx
Copy link

taralx commented Apr 22, 2019

I don't see why there would be a second switch - the socket-ness just has to be tracked by the libc (and probably should).

@rylev rylev added proposal A discussion about a specific actionable item wasi-filesystem Issues targeted for a `wasi_unstable_filesystem` module wasi-network-sockets Issues targeted for a possible `wasi_unstable_sockets` module labels May 8, 2019
@sunfishcode sunfishcode added the wasi-io Issues targeted for a `wasi_unstable_io` module label Sep 6, 2019
@syrusakbary
Copy link
Contributor

At @wasmerio we are very interested in moving sockets forward in WASI.

Are there any companies already working on this (or interested in working on it in the future), so we can start collaborating? (Mozilla? Fastly?)
I'd like to minimize any surprises when the spec advances, so Wasmer and all its users can use sockets as soon as the spec is ready.

Following that thought. I'd like to offer Wasmer workforce to start the development on the wasi-libc repo.

  1. Is everyone happy with @sunfishcode proposal for Networking in WASI?
  2. Is there any other thing that could block a PR once we start working on it?
  3. Would anyone like to collaborate with us in this effort?

sunfishcode added a commit to sunfishcode/WASI that referenced this issue Mar 2, 2020
Interesting bits:

 - End-of-stream (EOS, aka EOF) is indicated by returning the
  `$success_eos` errno value.

     - This differs from POSIX, where EOS is indicated by having `read`
       return 0, which is ambiguous in several cases. Of course, WASI
       libc will continue to provide POSIX semantics to C users.

     - This is a success condition being indicated with a non-zero errno
       value, which does not have precedent in POSIX.

 - `$fdstat`'s `$filetype` field is removed. The `$filetype` is still
   available through the `$filestat` type, though note that it's now
   protected by a rights flag. The goal is to start abstracting away more
   of the differences between files and sockets.

 - The `$peek` and `$waitall` flags are now available on `$read`, and can
   be used on files. This differs from POSIX.

 - POSIX says that `recv` and `send` fail if the stream is not a socket,
   but with `recv`/`send` merged into `read`/`write`, this restriction
   will no longer be automatically enforced. It could be manually enforced
   in WASI libc if needed, but in most cases, it's probably not important.

 - There is a new `$fd_line_oriented_terminal` right, to support
   `isatty`. I also anticipate expanding on this right in my upcoming
   terminal I/O proposal.

 - `$read`'s `$nread` result can be larger than the provided buffer to
   indicate a truncate message on a message-oriented socket (see also
   `$recv_data_truncated` in the current API, or `MSG_TRUNC` in POSIX
   `recv`. This avoids the need for a separate output value, but is
   it too surprising?

Out of scope for this proposal (but maybe in scope in the future!):

 - Streams vs datagrams - Having these merged is tricky, but this isn't
   new here; both `read` and `recv` already support both streams and
   datagrams.

 - Streams vs arrays - POSIX unifies the concept of a file as an array
   of bytes with the concept of file as a stream of data, which creates
   some tricky situations, but this also isn't new here.

 - Out-of-band messages and connectionless sockets aren't in WASI, and
   this proposal doesn't change that.

This implements WebAssembly#4.
@sunfishcode
Copy link
Member Author

This issue has been subsumed by the component model, which is adding a stream type which will provide read and write operations that we can use with all stream-like use cases.

hnpl pushed a commit to hnpl/libapps that referenced this issue Apr 11, 2023
WASI's sock_send and sock_recv are very similar to fd_write and fd_read,
but with an extra flag parameter. This CL makes sock_send/sock_recv
delegate to fd_write/fd_read if no flags are passed in.

See github.com/WebAssembly/WASI/issues/4 for additional context.

Bug: b/276352391
Change-Id: I52af0213bb928fb284adf6c74521cdaf451f10fc
Reviewed-on: https://chromium-review.googlesource.com/c/apps/libapps/+/4389201
Reviewed-by: Mike Frysinger <[email protected]>
Tested-by: kokoro <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal A discussion about a specific actionable item wasi-filesystem Issues targeted for a `wasi_unstable_filesystem` module wasi-io Issues targeted for a `wasi_unstable_io` module wasi-network-sockets Issues targeted for a possible `wasi_unstable_sockets` module
Projects
None yet
Development

No branches or pull requests

6 participants