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

wasi-http: make the buffer and budget capacity of the OutgoingBody writer configurable #9670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

iawia002
Copy link
Contributor

Please refer to issue #9653 for the background and reasons for the changes.

fix #9653

cc @alexcrichton @pchickey

Copy link
Contributor

@pchickey pchickey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, we never got back to making these buffer depths configurable in WasiHttpView but this is the right path.

Some style notes:

Ideally we'd rewrite the internals to just give the user a single bound on the amount buffered, rather than a combination of number of chunks and chunk size. If you are up for doing that, I believe its the best path forward, but it is trickier than what we have. (Wanting that ideal implementation is mostly why we didn't make this configurable yet.)

Instead of calling it writer_buffer call it outgoing_body_buffer_chunks, described as the number of distinct write calls to the outgoing body's output-stream that the implementation will buffer. write_budget should be outgoing_body_chunk_size, the maximum size allowed in a write call to the outgoing body's output-stream.

The minimum of 2 chunks is an implementation detail related to the way channel reservations work, lets make the default configuration 1, assert! that any value given is greater than 1, and then always add 1 when creating the channel because one empty slot is required by the way this implementation uses the channel.

Also, I don't think we should change the configuration in wasi-http/tests/all/main.rs, but instead fix the implementation of test_programs::http::request which has the exact same bug I pointed out in your guest in #9653 (comment). We should make sure that we have a tests that send a body exceeding the limits on both chunk size and buffer chunks.

@iawia002
Copy link
Contributor Author

Hi @pchickey, thanks for the detailed explanation and guidance, I think all comments addressed, PTAL

@pchickey
Copy link
Contributor

Thanks! Its a holiday weekend here and I’ll review this next week.

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.

wasi-http: can we call the blocking_write_and_flush method of the OutgoingRequest body multiple times?
2 participants