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

Handling/forwarding error-contexts that can be generated during a body.finish #153

Open
vados-cosmonic opened this issue Feb 10, 2025 · 5 comments

Comments

@vados-cosmonic
Copy link

During some implementation for error-contexts in futures in the wasip3-prototyping repo, we found that there was an issue with wasi:http/types#body.finish, as it implicitly does a future.read which can return a value or an error-context in the case of an error.

There are a few ways that we can solve this (thanks to @rvolosatovs and @dicej for suggestions), but a short incomplete list follows:

  1. Change the finish method signature from result<option<trailers>, error-code> to result<option<result<trailers, error-context>>,error-code>
  2. Absorb the error and convert it into an existing error-code (likely internal-error)
  3. Create a new error-code variant that stores an error-context

(1) is a reasonable approach but pollutes the type and possibly unreasonably forces caller to handle the error-context directly.

(2) causes some loss of context in an almost certain future version of error-context -- the best example is of an error context that can hold multiple traces (similar to an anyhow::Error), if we were to boil down the error context into only a string (given the current variants for error-code)

(3) Seems like an ideal option as a variation of (2) -- an additional error variant like internal-error-context(error-context) which would

There are of course other ways to handle this, so I'd like to open this up to discussion before submitting a PR to make the change (going for #3)

@vados-cosmonic vados-cosmonic changed the title Handling/forwarding error-contexts that can be forwarded during a body.finish Handling/forwarding error-contexts that can be generated during a body.finish Feb 10, 2025
@lukewagner
Copy link
Member

Great question!

In the medium term (in some 0.3.x release), one idea @sunfishcode had that I really like is that result should include an implicit error-context in the error case (as-if, thus result<T,U> would work at the ABI-level like result<T, tuple<U, error-context>>). (Since this would change the CABI of result, so we'd need to use some canonopt opt-in to preserve backwards compatibility during the transition.) If we had already made this change, then, because finish returns a result<..., error-code>, we could simply propagate the internal future's error-context along with the appropriate error-code.

In the short term, maybe it's fine to simply let the error-context drop, with the understanding that later we'll be able to propagate it? Otherwise, we could stick an error-context somewhere explicitly, but this will be something we'll want to deprecate later, so I'm not sure if it's worth the churn. Happy to hear other thoughts though.

@vados-cosmonic
Copy link
Author

Thanks for the feedback @lukewagner !

The idea of results always coming with an error-context is really interesting -- this makes sense to me in a world where error-context is the way the underlying system expresses errors (which is an interesting concept for many reasons).

One thing that makes me hesitate about that is the possible difficulty of intuiting that an error-context would come with every result/knowing what that was/forcing implementation of error-context to implement result. I think the WYSIWYG-ity of WIT as it stands now is really impressive (even if verbose at times), and being able to really easily map types to $LANGUAGE in the head of a new developer to the ecosystem is important -- and this might make that just a tad harder.

In the short term, maybe it's fine to simply let the error-context drop, with the understanding that later we'll be able to propagate it? Otherwise, we could stick an error-context somewhere explicitly, but this will be something we'll want to deprecate later, so I'm not sure if it's worth the churn. Happy to hear other thoughts though.

Yeah definitely want to hear what others think here. There are so many ways to go about this.

Another thing that I forgot to mention that @dicej brought up is that we could actually not preemptively fetch the trailers for people and return them to the users to decide what to do with. We're at such a low level that I think that this might be really compelling as well.

@vados-cosmonic
Copy link
Author

After chatting with @pchickey , the idea of allowing the caller to decide whether they want to listen for trailers seems to be the best option -- i.e. not pulling trailers inside the finish and instead returning a future<option<trailers>> that callers can use if they so choose.

Any thoughts @lukewagner ? I neglected to mention this suggestion in the first comment.

@lukewagner
Copy link
Member

@vados-cosmonic That sounds like a reasonable alternative interface, esp. if it obviates the root issue.

@vados-cosmonic
Copy link
Author

OK, got something up that implements this -- while I was going, I realized that the only case that seems reasonable for returning an error-code from body.finish is actually if you're resolving the trailers -- so it looks like we don't even need the outer result.

WIT change:
#154

Upstream impl:
https://github.com/bytecodealliance/wasip3-prototyping/pull/16/files

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

No branches or pull requests

2 participants