Skip to content
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

Merged

Conversation

iawia002
Copy link
Contributor

#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:

Screenshot 2024-05-21 at 17 21 56

/cc @alexcrichton @elliottt

@iawia002 iawia002 requested a review from a team as a code owner May 21, 2024 10:15
@iawia002 iawia002 requested review from alexcrichton and removed request for a team May 21, 2024 10:15
@bjorn3
Copy link
Contributor

bjorn3 commented May 21, 2024

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?

@iawia002
Copy link
Contributor Author

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.

@iawia002 iawia002 force-pushed the fix-invalid-socket-address branch from ede4253 to 326b512 Compare May 21, 2024 10:45
@alexcrichton
Copy link
Member

alexcrichton commented May 21, 2024

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?

@morenol
Copy link

morenol commented May 21, 2024

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?

@alexcrichton
Copy link
Member

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.

@iawia002 iawia002 force-pushed the fix-invalid-socket-address branch from 4f2cee2 to 909b68d Compare May 22, 2024 02:19
@iawia002
Copy link
Contributor Author

I have added a test for this case, following @elliottt's advice in #8676 (comment), PTAL @alexcrichton @elliottt

@github-actions github-actions bot added the wasi Issues pertaining to WASI label May 22, 2024
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@alexcrichton alexcrichton added this pull request to the merge queue May 22, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 22, 2024
@elliottt
Copy link
Member

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

@alexcrichton
Copy link
Member

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

@alexcrichton alexcrichton added this pull request to the merge queue May 22, 2024
Merged via the queue into bytecodealliance:main with commit f1fe2af May 22, 2024
36 checks passed
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request May 22, 2024
…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 <alex@alexcrichton.com>
alexcrichton added a commit that referenced this pull request May 22, 2024
…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 <z2d@jifangcheng.com>
@iawia002 iawia002 deleted the fix-invalid-socket-address branch May 23, 2024 00:36
alexcrichton added a commit to alexcrichton/wasmtime that referenced this pull request May 31, 2024
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wasi Issues pertaining to WASI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants