Accept more Status codes in get_property. #237
Conversation
|
@martijneken will you be managing this moving forward? |
src/hostcalls.rs
Outdated
| Status::NotFound | Status::Empty => Ok(None), | ||
| Status::SerializationFailure | ||
| | Status::BadArgument | ||
| | Status::BadExpression | ||
| | Status::Unimplemented => Err(status), |
There was a problem hiding this comment.
This is a blind shot at trying to guess what "could" happen. I'm of the opinion that opening a seam to let the SDK user deal with them would be a more desirable way... if only "not the default" path so to keep the additional value of making it easier for implementers to test their host.
Unsure what the best approach would be? There could also be some global "flag" or something to get to a more lenient runtime behavior which lets the user take on the responsibility of dealing with "less standard" behaviors rather than having the SDK panic maybe?
If it is somewhere documented what valid Status every host method can return, I'm more than happy to generalize the pattern across the board.
Or would alternate behaviors be encouraged to be supported by using a tweaked copy of this SDK instead?
There was a problem hiding this comment.
If it is somewhere documented what valid
Statusevery host method can return, I'm more than happy to generalize the pattern across the board.
This pattern is already in this SDK, with some missing values (as you found out).
Or would alternate behaviors be encouraged to be supported by using a tweaked copy of this SDK instead?
What alternative behaviors do you have in mind, exactly?
There was a problem hiding this comment.
What alternative behaviors do you have in mind, exactly?
A path where the client code could gracefully deal with the "out of spec" (based on the above, that'd be any host behaving differently than the current Envoy codebase tho).
panic! in a crate is some radical thing to do. While I do see the advantage for this being the "default" behavior indeed, having some way to dealing gracefully with the Err would be nice I think... So unsure how to best enable that, wdyt? Would you agree that it'd be a nice to have? If so, how? make the unsafe method pub maybe and let someone deal with the entirety of the "problem"? Have some alternate method that gets you access to the raw Result? Possibly layering the code, where the "default public API" would call into the other pub fn, but deal explicitly with the Err behavior?
There was a problem hiding this comment.
I understand what are you suggesting, but I'm still wondering who would ever use that and why... What host is expected to be knowingly out of spec and for what purpose?
There was a problem hiding this comment.
We would use this, if only just on this very issue. Today we are stuck waiting on merge and release. So we need to fork in order to be able to make progress. I don't think (or say hope) any host would ever knowingly go out of spec... but the spec is loosely defined and requires one to look at Envoy if I go by how this was addressed. Envoy 1.20 (i.e. the proxy, the host - and probably more versions) crashed on SerializationFailure... All that to say that mistakes happen and hopefully also then get fixed, but until that happens it is, if only for us - that leverage this as part of bigger project then used by others - panicking is a very radical thing to do. So while I agree that hopefully we can find these issue easily while testing because of that very current behavior, being able to then also gracefully deal with them is desirable from a UX perspective.
Signed-off-by: Alex Snaps <alex@wcgw.dev>
|
Is there some next release on the horizon that could contain this (and the other changes, possibly more in the pipe)? |
Yes! This or more likely next week. |
…roperty Signed-off-by: Alex Snaps <alex@wcgw.dev>
PiotrSikora
changed the title
Signed-off-by: Alex Snaps <alex@wcgw.dev>
Adds the missing
Statusenum variants...We currently run into
SerializationFailurewhen getting some attributefilter_statefrom Envoy... Today, this not only panics, but the error isn't even really recognized.I'm not sure, given the current
trait Contextdefinition how to best deal with these gracefully tho? The default implementation are "just"Result::unwrap()ing the underlying host call.. So thinking that this is the best for now, wdyt?