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

feat: allow independent retrieval of trailers #154

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

vados-cosmonic
Copy link

@vados-cosmonic vados-cosmonic commented Feb 19, 2025

This commit changes wasi:http/types#body.finish to forward the optionally present trailers, rather than retrieve them with the possibility of generating an error along the way.

With this change, callers of body.finish will have to retreive trailers on their own, which may produce an efficiency gain in the case where trailers are present but not needed, and avoiding work of checking for trailers in the first place.

👀 Implementation preview

As this is a WASI p3 feature (for which work is ongoing in the wasip3-prototyping repository), you can "preview" the effects of this change on the implementation in the PR there.

This commit changes wasi:http/types#body.finish to forward the
optionally present trailers, rather than retrieve them with the
possibility of generating an error along the way.

With this change, callers of `body.finish` will have to retreive
trailers on their own, which *may* produce an efficiency gain in the
case where trailers are present but not needed, and avoiding work of
checking for trailers in the first place.

Signed-off-by: Victor Adossi <[email protected]>
@vados-cosmonic vados-cosmonic force-pushed the refactor=require-independent-retrieval-of-body-trailers branch from 272f039 to beed353 Compare February 19, 2025 21:28
@dicej
Copy link
Collaborator

dicej commented Feb 19, 2025

@vados-cosmonic I'm wondering if we should stick with the option<future<trailers>> param type in the body constructor. I realize it's asymmetric with respect to body.finish, but it's arguably ideal from an ergonomics standpoint given that most applications will neither send nor receive trailers. When creating a new body without trailers, the easiest thing to do is specify none rather than go through the motions of creating a new future just to send none to it (or close it without sending anything, which I suppose would be morally equivalent).

Obviously, the asymmetry implies that the implementation will need to transpose a option<future<trailers>> to a future<option<trailers>> in some scenarios, but I expect that won't be difficult.

@dicej
Copy link
Collaborator

dicej commented Feb 19, 2025

Not to complicate things further, but we could even make body.finish return a future<trailers> and assume the write end will close the future without sending anything in the case there are no trailers.

@vados-cosmonic
Copy link
Author

vados-cosmonic commented Feb 20, 2025

Hey @dicej waht do you think about actually changing the constructor for body to one without trailers and separating the ability to add/set trailers (either a different constructor or a setter are both fine).

Obviously, the asymmetry implies that the implementation will need to transpose a option<future> to a future<option> in some scenarios, but I expect that won't be difficult.

So I found this difficult to do in the impl without creating another future during finish -- transposing the Option into the FutureReader goes across the async boundary and the types don't quite permit me to change the Tto an Option<T> whether it's in the host machinery or otherwise. Maybe I was missing something obvious!

Not to complicate things further, but we could even make body.finish return a future and assume the write end will close the future without sending anything in the case there are no trailers.

This is certainly an interesting idea -- does this make it a little more difficult in that the write end has to remember to close the future? If this introduces the possibility of a hang due to incorrectly implemented writers it might increase the hurdle.

This also kind of makes me want to separate the common case and the with-trailers case into different functions, what do you think?

@pchickey if you want to chime in on the expanded interface changes (separating with trailers functions out), would appreciate it!

@dicej
Copy link
Collaborator

dicej commented Feb 20, 2025

So I found this difficult to do in the impl without creating another future during finish -- transposing the Option into the FutureReader goes across the async boundary and the types don't quite permit me to change the Tto an Option<T> whether it's in the host machinery or otherwise. Maybe I was missing something obvious!

Yeah, you'd need to create another future, although I think we can avoid that if finish returns future<trailers> instead of future<option<trailers>>.

This is certainly an interesting idea -- does this make it a little more difficult in that the write end has to remember to close the future?

They will either need to remember to send none or close the future, and I don't think the latter is more onerous than the former.

@dicej
Copy link
Collaborator

dicej commented Feb 20, 2025

BTW, I'm also fine with removing the trailers parameter from the constructor and e.g. adding a new new-with-trailers: static func(%stream: stream<u8>, trailers: future<trailers>) -> body; function.

@vados-cosmonic
Copy link
Author

vados-cosmonic commented Feb 20, 2025

Yeah, you'd need to create another future, although I think we can avoid that if finish returns future instead of future<option>.

I'm in favor of this!

BTW, I'm also fine with removing the trailers parameter from the constructor and e.g. adding a new new-with-trailers: static func(%stream: stream, trailers: future) -> body; function.

A tiny nit, but I personally like just with-trailers (but given that new is a thing, consistency is probably better here).

Will re-write the impl and make updates here with that in mind.

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

Successfully merging this pull request may close these issues.

3 participants