Stream: git-wasmtime

Topic: wasmtime / PR #9670 wasi-http: make the buffer and budget...

Wasmtime GitHub notifications bot (Nov 25 2024 at 06:00):

iawia002 opened PR #9670 from iawia002:wasi-http-writer-buffer to bytecodealliance:main:

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

fix #9653

cc @alexcrichton @pchickey

Wasmtime GitHub notifications bot (Nov 25 2024 at 06:00):

iawia002 requested pchickey for a review on PR #9670.

Wasmtime GitHub notifications bot (Nov 25 2024 at 06:00):

iawia002 requested wasmtime-core-reviewers for a review on PR #9670.

Wasmtime GitHub notifications bot (Nov 27 2024 at 20:18):

pchickey submitted PR review:

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.

Wasmtime GitHub notifications bot (Nov 27 2024 at 20:21):

pchickey submitted PR review:

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 https://github.com/bytecodealliance/wasmtime/issues/9653#issuecomment-2504699703.

Wasmtime GitHub notifications bot (Nov 27 2024 at 20:23):

pchickey submitted PR review:

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 https://github.com/bytecodealliance/wasmtime/issues/9653#issuecomment-2504699703. We should make sure that we have a tests that send a body exceeding the limits on both chunk size and buffer chunks.

Wasmtime GitHub notifications bot (Nov 28 2024 at 02:24):

iawia002 updated PR #9670.

Wasmtime GitHub notifications bot (Nov 28 2024 at 02:29):

iawia002 commented on PR #9670:

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

Wasmtime GitHub notifications bot (Nov 28 2024 at 08:53):

iawia002 updated PR #9670.

Wasmtime GitHub notifications bot (Nov 28 2024 at 16:45):

pchickey commented on PR #9670:

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

Wasmtime GitHub notifications bot (Dec 04 2024 at 21:36):

alexcrichton submitted PR review:

Looks reasonable to me, thanks! (Pat's taking a break for a bit)

I think this is reasonable enough that we can follow-up with further iterations if necessary.

Wasmtime GitHub notifications bot (Dec 04 2024 at 21:36):

alexcrichton has enabled auto merge for PR #9670.

Wasmtime GitHub notifications bot (Dec 04 2024 at 21:37):

alexcrichton submitted PR review:

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 https://github.com/bytecodealliance/wasmtime/issues/9653#issuecomment-2504699703. We should make sure that we have a tests that send a body exceeding the limits on both chunk size and buffer chunks.

Wasmtime GitHub notifications bot (Dec 04 2024 at 21:53):

alexcrichton merged PR #9670.

Wasmtime GitHub notifications bot (Dec 04 2024 at 22:41):

iawia002 commented on PR #9670:

Thanks for the review, I’m currently on vacation, I will intergrate

Wasmtime GitHub notifications bot (Dec 04 2024 at 22:43):

iawia002 edited a comment on PR #9670:

Thanks for the review, I’m currently on vacation, I will integrate this into wasmtime cli on a few days.

Last updated: Dec 23 2024 at 13:07 UTC