Stream: git-wasmtime

Topic: wasmtime / PR #8085 Fix `connect_timeout` for `wasi-http`

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 08:01):

rikhuijzer opened PR #8085 from rikhuijzer:rh/wasi-http-timeout to bytecodealliance:main:

@elliottt in https://github.com/bytecodealliance/wasmtime/issues/7356 mentions that connect_timeout, first_byte_timeout, and between_bytes_timeout should be tested.

This PR adds a test for connect_timeout and ensures that TcpStream::connect adheres to the timeout. Without this fix, setting connect_timeout has no effect on hosts where TcpStream::connect tries to connect.

Note that the variable connect_timeout is also used a bit later for the handshake:

https://github.com/bytecodealliance/wasmtime/blob/8dd2ae9e2c57b3fd32f91fb8f0ec1543854f4c7c/crates/wasi-http/src/types.rs#L207-L213

I'm not sure whether using this variable twice is correct.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 08:01):

rikhuijzer requested alexcrichton for a review on PR #8085.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 08:01):

rikhuijzer requested wasmtime-core-reviewers for a review on PR #8085.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 08:02):

rikhuijzer updated PR #8085.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 08:28):

rikhuijzer updated PR #8085.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 14:00):

rikhuijzer updated PR #8085.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 17:13):

elliottt submitted PR review:

This looks great, thank you!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 17:13):

elliottt submitted PR review:

This looks great, thank you!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 17:13):

elliottt created PR review comment:

Thanks for matching the timeout explicitly here!

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 17:16):

elliottt commented on PR #8085:

I'm not sure whether using this variable twice is correct.

I see two uses in the handler function, but they're under different branches of the same conditional. Was there another use of connect_timeout that you were thinking of?

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 17:37):

rikhuijzer commented on PR #8085:

This looks great, thank you!

Thanks for the review!

I'm not sure whether using this variable twice is correct.

I see two uses in the handler function, but they're under different branches of the same conditional. Was there another use of connect_timeout that you were thinking of?

There should be three (https://github.com/bytecodealliance/wasmtime/blob/eebce11eb1fdfc67ab8fc7a06cb68ba1231dd7a6/crates/wasi-http/src/types.rs).

One is now newly added near the TcpStream::connect and the other two are inside the branches near hyper::client::conn::http1::handshake. I think these two different phases represent TCP's SYN and ACK.

Why I mentioned it is because it sounded a bit weird to me that the connect_timeout is used twice, but on second thought maybe it's fine. The SYN and ACK are both still during the "Connection establishment" phase.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 17:47):

elliottt commented on PR #8085:

Ah! Sorry for misunderstanding. I think this is fine to leave it this way, but I agree that it's a little odd. Reading through the handshake implementation, it's assuming that the connection has already been made, so if anything we might want to remove the timeout on the second two uses in the function.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 17:52):

elliottt commented on PR #8085:

I added #8104 to record the fact that we should look into removing the uses of connect_timeout on the handshake calls, but that issue won't be valid until this PR merges.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 18:22):

elliottt submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 18:22):

elliottt created PR review comment:

This check failed on the MinGW build. What do you think about removing it and adding it back in a follow-up PR? As an aside, you can add prtest:mingw-x64 in a commit message to explicitly run that job.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 18:42):

rikhuijzer updated PR #8085.

view this post on Zulip Wasmtime GitHub notifications bot (Mar 12 2024 at 20:09):

elliottt merged PR #8085.

Last updated: Dec 23 2024 at 12:05 UTC