Turn request-options into a resource
#7417
Conversation
elliottt
requested review from
alexcrichton and
pchickey
and removed request for
a team
There was a problem hiding this comment.
Looks good to me.
Random drive-by thought unrelated to this change specifically, could the *-ms bits be updated to take the duration type in #7358 which would give more precision here as well?
crates/wasi-http/src/types.rs
Outdated
| @@ -257,6 +257,13 @@ pub struct HostOutgoingRequest { | |||
| pub body: Option<HyperOutgoingBody>, | |||
| } | |||
|
|
|||
| #[derive(Default)] | |||
| pub struct HostRequestOptions { | |||
| pub connect_timeout: Option<u32>, | |||
There was a problem hiding this comment.
Lets use either std or tokio::time::Duration to represent these values in the host as early as we can.
There was a problem hiding this comment.
The downside is that the getters become quite fallible as they're turning a std::time::Duration back into a u32.
There was a problem hiding this comment.
Isnt that resolved when we switch to using a monotonic duration, which is a u64?
There was a problem hiding this comment.
std::time::Duration::as_millis returns a u128, so we won't avoid it completely.
There was a problem hiding this comment.
I implemented this by treating conversion failure from u128 as an Ok(None) return value. Since we default all these timeouts to None anyway, I think a conversion failure would mean that the guest somehow managed to set a larger than u32 value to begin with, then tried to get one back. The host setting an invalid default would be the other possibility, but we don't do this anywhere in wasmtime currently.
Yes, we can make that change as soon as 7358 lands. |
|
You can merge with |
elliottt
force-pushed
the
trevor/request-options-resource
branch
from
e6e46b9
to
711e1f4
Compare
As we'd like to be able to modify
request-optionsin the future without breaking compatibility with older guests, turnrequest-optionsinto a resource. This allows us to expose getters and setters instead of fields on a record, which gives the embedder flexibility in what additional options are allowed.Fixes #7409