Stream: git-wasmtime

Topic: wasmtime / PR #8671 wasi-http: add the port to authority ...

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 10:15):

iawia002 opened PR #8671 from iawia002:fix-invalid-socket-address to bytecodealliance:main:

<!--
Please make sure you include the following information:

Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.html

Please ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->

#8563 removes the port from the authority, it makes the target address cannot be correctly resolved when opening a TCP connection. This results in that if we want to send an HTTP request in the wasi component, an error ErrorCode::ConnectionRefused will be reported directly. The underlying error is invalid socket address:

<img width="521" alt="Screenshot 2024-05-21 at 17 21 56" src="https://github.com/bytecodealliance/wasmtime/assets/9134003/6473efef-4306-45c7-8ca4-e5101d14f071">

/cc @alexcrichton @elliottt

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 10:15):

iawia002 requested alexcrichton for a review on PR #8671.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 10:15):

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

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 10:24):

bjorn3 commented on PR #8671:

What if the program explicitly specified a port different from 80 or 443? Wouldn't this change break that by creating a nonsensical address like 127.0.0.1:1234:80 when trying to request http://127.0.0.1:1234/some_page?

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 10:28):

iawia002 commented on PR #8671:

What if the program explicitly specified a port different from 80 or 443? Wouldn't this change break that by creating a nonsensical address like 127.0.0.1:1234:80 when trying to request http://127.0.0.1:1234/some_page?

Indeed, I didn't consider that. I will update it again.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 10:45):

iawia002 updated PR #8671.

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 18:35):

alexcrichton commented on PR #8671:

Does this mean that default_send_request_handler basically doesn't work by default? If so, that seems bad!

Would you be up for adding a test for this?

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 18:35):

alexcrichton edited a comment on PR #8671:

Does this mean that default_send_request_handler basically previously didn't work by default? If so, that seems bad!

Would you be up for adding a test for this?

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 20:33):

morenol commented on PR #8671:

Does this mean that default_send_request_handler basically previously didn't work by default? If so, that seems bad!

Would you be up for adding a test for this?

I think that that is the case, can we have a fix like this one on a 21.0.1 version?

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 20:40):

morenol edited a comment on PR #8671:

Does this mean that default_send_request_handler basically previously didn't work by default? If so, that seems bad!

Would you be up for adding a test for this?

I think that that is the case, could we have a fix like this one on a 21.0.1 version?

view this post on Zulip Wasmtime GitHub notifications bot (May 21 2024 at 20:47):

alexcrichton commented on PR #8671:

Yeah, definitely seems bad enough for a point release! Would you be up for adding a test here? perhaps fetching something like example.com or similar? That'll be flaky over time but we can try to add various exceptions for possible errors due to flakiness over time.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 02:11):

iawia002 updated PR #8671.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 02:19):

iawia002 updated PR #8671.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 02:21):

iawia002 commented on PR #8671:

I have added a test for this case, following @elliottt's advice in https://github.com/bytecodealliance/wasmtime/pull/8676#discussion_r1609131895, PTAL @alexcrichton @elliottt

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 15:37):

alexcrichton submitted PR review:

Thanks!

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 15:57):

elliottt commented on PR #8671:

Could you remove the use of ssl in your test program? CI failed because it's not supported on riscv64.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 15:59):

alexcrichton updated PR #8671.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 15:59):

alexcrichton has enabled auto merge for PR #8671.

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 15:59):

alexcrichton commented on PR #8671:

Ah I went ahead and just pushed up a commit to ignore s390x/riscv64, I don't think it's too critical to get all the architectures tested here

view this post on Zulip Wasmtime GitHub notifications bot (May 22 2024 at 16:25):

alexcrichton merged PR #8671.

Last updated: Nov 22 2024 at 16:03 UTC