Added socket send and recv timeout options #1419
Conversation
| } | ||
|
|
||
| // TODO: Replace this with posix sockopts when available | ||
| #ifdef __wasi__ |
There was a problem hiding this comment.
We'd like to use the original API and translate them to Wasm. Users should not learn another set of APIs to develop Wasm application
Since timeout is an opt of a socket, SO_SNDTIMEO, using setsockopt(...) and getsockopt(..) is better.
There was a problem hiding this comment.
I think this is just an intermediate step and it will be replaced with getsockopt() and secksockopt() once we have it implemented in wasi_socket_ext.c, is that right, @cimacmillan ?
There was a problem hiding this comment.
That was my original plan, but I'll raise a revision for it as part of this PR if that's preferred
|
@loganek Could you please help review the PR? |
| } | ||
|
|
||
| // TODO: Replace this with posix sockopts when available | ||
| #ifdef __wasi__ |
There was a problem hiding this comment.
I guess we could already add #else branch because (get/set)sockopt is available for non-wasi platform.
| { | ||
| return (__wasi_errno_t) | ||
| __imported_wasi_snapshot_preview1_sock_set_recv_timeout( | ||
| (int32_t)fd, (int32_t)timeout_us); |
There was a problem hiding this comment.
any reason we convert timeout_us to int32_t and not keeping it 64-bit variable? Wasm supports both 32-bit and 64-bit variables. See https://webassembly.github.io/spec/core/syntax/types.html
There was a problem hiding this comment.
Here I'm converting the timeout_us pointer to int32_t, rather than passing the value as int64. It's because of this limitation:
export_native_api.md - As function parameters are always passed in 32 bits numbers, you can also use 'i' for the pointer type argument, then you must do all the address conversion and boundary checking in your native function.
Tested that passing ULLONG_MAX (rounded down to nearest 1000) sets and receives properly. There should probably be a specific test case for this though
There was a problem hiding this comment.
Doesn't that refer to pointer types? I think it's possible to pass int64, that's why we have I (uppercase i) as a registration parameter. You can also see some examples in wasi libc, e.g. fd_pwrite's offset parameter is of wasi_filesize_t type which is a type alias of uint64_t.
There was a problem hiding this comment.
Yeah you're right I can use I, misunderstood this part. Will update
| } | ||
|
|
||
| // TODO: Replace this with posix sockopts when available | ||
| #ifdef __wasi__ |
There was a problem hiding this comment.
I think this is just an intermediate step and it will be replaced with getsockopt() and secksockopt() once we have it implemented in wasi_socket_ext.c, is that right, @cimacmillan ?
|
|
||
| curfds = wasi_ctx_get_curfds(module_inst, wasi_ctx); | ||
|
|
||
| return wasmtime_ssp_sock_set_recv_timeout(curfds, fd, *timeout_us); |
There was a problem hiding this comment.
before passing the timeout_us further, we need to check do check the memory using validate_native_addr(). See usage of that macro in this file for examples.
| HANDLE_ERROR(error); | ||
| *(struct timeval *)optval = time_us_to_timeval(timeout_us); | ||
| return error; | ||
| break; |
There was a problem hiding this comment.
there's no need for break if you already have return one line above that.
There was a problem hiding this comment.
Forgot to remove, thanks for catching
|
|
||
| switch (level) { | ||
| case SOL_SOCKET: | ||
| switch (optname) { |
There was a problem hiding this comment.
switch inside another switch makes the code a bit less readable, consider refactoring the code to avoid deeply nested code.
There was a problem hiding this comment.
Agree, maybe separate functions depending on the level? What'd you recommend?
There was a problem hiding this comment.
I think splitting it into functions is not a bad idea. Also, I don't think we're going to add support to level different than SOL_SOCKET, so you might consider failing early at the beginning of the function:
if (level != SOL_SOCKET) {
return -1;
}
There was a problem hiding this comment.
Thanks, will split because I was planning to include a couple IPPROTO_TCP ones
cimacmillan
force-pushed
the
cmmacmil/timeout
branch
from
80940cf to
54f4f1d
Compare
Added socket send and recv timeout options with implementation for posix platform. This is part of a extending support for sockets in WASI. bytecodealliance#1336. Also add sample that sets and reads back the send and receive timeouts using the native function binding.
4 participants
Added socket send and recv timeout options with implementation for posix platform
This is part of a extending support for sockets in WASI. #1336
I've also added a WIP sample that sets and reads back the send and receive timeouts using the native function binding, however I still need to add the posix-like shim.