Unify more of the p2/p3 paths in the wasmtime-wasi-http crate#13812
Conversation
| } | ||
| } | ||
|
|
||
| warn!("hyper error: {err:?}"); |
There was a problem hiding this comment.
I'm a bit surprised to see warn-level logging in a From implementation. What's the rationale?
(same comment applies to p3/conv.rs)
There was a problem hiding this comment.
This is pseudo-preserving preexisting behavior where this is a copy of this function for example, although it wasn't modeled as From before.
IIRC though the source of this is me asking to add a log here. I'm personally paranoid about dropping error context on the floor as it can create pretty confusing situations downstream, and this was intended to strike a balance of "someone can figure things out if things go wrong" with the representation needs here.
I do agree though it's a bit different being in From vs a freestanding function. Do you have a preference to model this not as From but with an explicit conversion?
There was a problem hiding this comment.
I don't feel strongly about it, although making it an explicit conversion seems reasonable; most people aren't expecting From/Into to have side effects like logging.
There was a problem hiding this comment.
Another idea: implement TryFrom instead of From.
This commit starts paving the path to having wasip2 and wasip3 for HTTP use the same context, hook trait, etc. The main piece introduced here is a new `wasmtime_wasi_http::Error` type which is intended to unify the wasip2 and wasip3 error codes while also supporting other typed errors. This does not yet update wasip2 and only updates the wasip3 side of things. Much of the wasip3 changes are juggling errors types where everything coming from the host is an `Error` while everything related to the guest is an `ErrorCode`. Locations were adjusted as appropriate to convert where needed.
Also get all the tests passing
No need for so much error conversion now that there's one crate type
Leave this to a helper function for now.
Similar to the previous commit, but required a bit more juggling.
|
Ok @dicej mind taking another look at the recent commits? |
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
2 participants
This PR takes us further down the road of designing the wasmtime-wasi-http crate in the same manner as the wasmtime-wasi crate which is to say that there's one source-of-truth context/configuration/etc which all WASI impls are derived from. Previously there were separate hooks/contexts for p2/p3 due to historical differences, and this is a refactoring to bring the two together.
The largest part of the refactoring here is the addition of a new
wasmtime_wasi_http::Errortype which unifies p2/p3 as well as underlying possible errors. This means that host-facing operations now work with this type rather than the raw WIT types.Some pieces this helps are:
default_send_requesthandler.rsmoduleNote that this is currently built on #13810