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

implement basic TCP/UDP client support #477

Merged
merged 2 commits into from
Mar 13, 2024

Conversation

dicej
Copy link
Contributor

@dicej dicej commented Feb 28, 2024

This implements socket, connect, recv, send, etc. in terms of wasi-sockets for the wasm32-wasi-preview2 target.

I've introduced a new public header file: __wasi_snapshot.h, which will define a preprocessor symbol __wasilibc_use_preview2 if using the wasm32-wasi-preview2 version of the header, in which case we provide features only available for that target.

libc-bottom-half/headers/public/__wasi_snapshot.h Outdated Show resolved Hide resolved
return -1;
} else {
return count;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My current understanding is that we should also call streams_method_output_stream_flush here, to best implement POSIX send semantics where, when send returns successfully, it has actually transferred the number of bytes it has claimed to have transferred.

flush in Wasmtime's tcp implementation is a no-op, and it seems likely to be a no-op in other implementations as well, so it shouldn't be that expensive to call it here.

Copy link
Contributor

@badeend badeend Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

implement POSIX send semantics where, when send returns successfully, it has actually transferred the number of bytes it has claimed to have transferred.

That's not quite right. When send returns successfully, it doesn't mean anything. I tried to capture this in WebAssembly/wasi-io#73

the OS' non-blocking send implementation doesn't perform any IO itself. Most likely all it does is move the data from user space to a kernel queue for it to be put on the wire at a later moment. If send returns an error, it's likely that it has nothing to do with the current send call, but instead it is a delayed notification that one of the previous sends failed. Or put differently; if send returns success, it doesn't mean the data was successfully sent. It only means ownership of the buffer has been transferred from the application to the kernel.

IMO, filesystem fds and internet socket fds are different beasts entirely. Sockets are already stream-like by nature. The fact that write on filesystem descriptors need flushing after every write is because WASI exposes the file's contents as a buffered stream, even though POSIX promises consistency between other readers/writers on that same file. Hence the flush.

I don't have many examples nor counterexamples, but I'd say that within the concept of "streams" in general, the filesystem's behavior of "flush every write" is more an exception than the norm.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My concern here is not about "is the write visible to anyone" or "has the peer received the bytes". This is just about "would POSIX expect us to report an error from this send call?". Calling flush should ensure that if the implementation has an ECONNRESET waiting for us that a synchronous send would have observed, that we observe it too.

One could argue that if we don't observe it in the current send call, we'll just report it in the next send call, and that that's acceptable. And maybe it is; my thought was just to err on the side of caution for now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just about "would POSIX expect us to report an error from this send call?".

How would this work on non-blocking sockets?

libc-bottom-half/sources/recv.c Show resolved Hide resolved
libc-bottom-half/sources/sockets_utils.c Show resolved Hide resolved
@jeffparsons
Copy link

Am I correct in thinking that @sunfishcode has outstanding concerns here? (Just trying to figure out the status of this PR / where the ball is.)

@dicej
Copy link
Contributor Author

dicej commented Mar 11, 2024

Am I correct in thinking that @sunfishcode has outstanding concerns here? (Just trying to figure out the status of this PR / where the ball is.)

Yeah, IIUC @sunfishcode thinks maybe send should flush to conform to POSIX error reporting expectations and @badeend thinks that's unnecessary overhead. I'm fine either way and will do whatever's necessary to get this merged. @badeend perhaps I could add the flush for now and we can revisit later just to keep this process moving?

@badeend
Copy link
Contributor

badeend commented Mar 13, 2024

IIUC @sunfishcode thinks maybe send should flush to conform to POSIX error reporting expectations and @badeend thinks that's unnecessary overhead.

I'm not worried about performance. The point I'm trying to get across is that, IMO, not flushing is the desired POSIX semantics. Waiting around for a potential error (through flushing) would break the non-blocking-ness of non-blocking sockets.

@sunfishcode
Copy link
Member

I think I'm not confident in the flush situation yet, however I'm thinking that we can merge this PR as-is, once the conflicts are resolved, and figure out the right flush usage over time.

dicej and others added 2 commits March 13, 2024 11:51
This implements `socket`, `connect`, `recv`, `send`, etc. in terms of
`wasi-sockets` for the `wasm32-wasip2` target.

I've introduced a new public header file: `__wasi_snapshot.h`, which will define
a preprocessor symbol `__wasilibc_use_wasip2` if using the `wasm32-wasip2`
version of the header, in which case we provide features only available for that
target.

Co-authored-by: Dave Bakker <github@davebakker.io>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
Co-authored-by: Dan Gohman <dev@sunfishcode.online>
Signed-off-by: Joel Dice <joel.dice@fermyon.com>
@dicej dicej force-pushed the wasi-sockets-basic-client branch from 1145d85 to 6188a60 Compare March 13, 2024 17:52
@sunfishcode sunfishcode merged commit 684f155 into WebAssembly:main Mar 13, 2024
8 checks passed
@sunfishcode
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants