Fix connect_timeout for wasi-http

[rikhuijzer]

@elliottt in #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:

wasmtime/crates/wasi-http/src/types.rs

Lines 207 to 213 in 8dd2ae9

	let (sender, conn) = timeout(
	connect_timeout,
	hyper::client::conn::http1::handshake(stream),
	)
	.await
	.map_err(|_| types::ErrorCode::ConnectionTimeout)?
	.map_err(hyper_request_error)?;

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

[elliottt]

This looks great, thank you!

[elliottt]

Thanks for matching the timeout explicitly here!

[elliottt]

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?

[rikhuijzer]

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.

[elliottt]

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.

[elliottt]

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.