-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: add the port to authority when opening a TCP connection #8671
wasi-http: add the port to authority when opening a TCP connection #8671
Conversation
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 |
Indeed, I didn't consider that. I will update it again. |
ede4253
to
326b512
Compare
Does this mean that 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? |
Yeah, definitely seems bad enough for a point release! Would you be up for adding a test here? perhaps fetching something like |
326b512
to
4f2cee2
Compare
4f2cee2
to
909b68d
Compare
I have added a test for this case, following @elliottt's advice in #8676 (comment), PTAL @alexcrichton @elliottt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Could you remove the use of ssl in your test program? CI failed because it's not supported on riscv64. |
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 |
…ytecodealliance#8671) * wasi-http: add the port to authority when opening a TCP connection * Ignore test on riscv64 and s390x --------- Co-authored-by: Alex Crichton <[email protected]>
…8671) (#8678) * wasi-http: add the port to authority when opening a TCP connection * Ignore test on riscv64 and s390x --------- Co-authored-by: Xinzhao Xu <[email protected]>
This handles flakiness [seen online] and makes the test more robust. It was known that this test was not "fully robust" when added in bytecodealliance#8671 and this is the "work over time" to handle failures we see in the wild. [seen online]: https://github.com/bytecodealliance/wasmtime/actions/runs/9324271722/job/25669233977
This handles flakiness [seen online] and makes the test more robust. It was known that this test was not "fully robust" when added in #8671 and this is the "work over time" to handle failures we see in the wild. [seen online]: https://github.com/bytecodealliance/wasmtime/actions/runs/9324271722/job/25669233977
#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 isinvalid socket address
:/cc @alexcrichton @elliottt