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

vlib: fix tcp_test.v #20389

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

vlib: fix tcp_test.v #20389

wants to merge 1 commit into from

Conversation

hholst80
Copy link
Contributor

@hholst80 hholst80 commented Jan 4, 2024

The listen_tcp method should not accept .unix address family.

Test that it indeed fails when invoked.

Also, remove the flaky flag on tcp_test.v as none of the tests here has any real reason to be flaky.

Tested on Linux 6.6 (amd64) and FreeBSD 13.2 (amd64).

@hholst80 hholst80 force-pushed the fix-tcp-tests branch 2 times, most recently from de5f494 to 46adaac Compare January 4, 2024 21:55
@JalonSolov
Copy link
Contributor

Panics on Windows... maybe part of why it was marked flaky...

@Casper64
Copy link
Member

Casper64 commented Jan 4, 2024

0.0.0.0:45123: net: op timed out; code: 9

Why is it trying to dial to an ipv4 address when an ipv6 address is given? Seems like a bug in the address resolve code to me

@hholst80
Copy link
Contributor Author

hholst80 commented Jan 4, 2024

0.0.0.0:45123: net: op timed out; code: 9

Why is it trying to dial to an ipv4 address when an ipv6 address is given? Seems like a bug in the address resolve code to me

I found some funky looking code in tcp.c.v that I removed. It was responsible for replacing ::: with localhost which resolved to an IP4 address. Having a generic resolve which magically figures out the address family is probably not very safe, but yet common. I've ran into a similar issue with Node.js too.

Comment on lines 106 to 107
mut conn := net.dial_tcp_with_bind('vlang.io:80', '127.0.0.1:0')!
conn.close()!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

How could this ever have worked? test_bind() is not declared with ! so I thought conn.close()! would be illegal (nothing is returned)?

Copy link
Member

Choose a reason for hiding this comment

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

test_ functions act similar to main. They are both exception for the general rule, that functions should themselves return ! or ?, if you want to use f()! or f()? inside them.

The reasoning for that, is that both main() and the test_...() functions are not called by V code themselves, but by generated code, so there is nowhere for the propagation to go, and instead in these top level functions, propagation just panics on the spot.

It also makes them a bit easier to write, since you only have to remember fn main() {, instead of having to remember to change it to fn main() ! { or fn main() ? {.

Copy link
Member

Choose a reason for hiding this comment

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

Personally, I also dislike this decision, even when I know the motivation and reasons behind it.

In my opinion, having all functions behave uniformly, without syntax exceptions, would have been more consistent and easier to remember 🤷🏻‍♂️ .

@hholst80
Copy link
Contributor Author

hholst80 commented Jan 5, 2024

OK. That leads into a rabbit hole. What does not work on Windows is to open a connection to 0.0.0.0. That is only a valid IP address to bind to for accept. If we know for sure that the server is listening on all devices on a specific port, then we know for sure that it is listening on 127.0.0.1 (within reason). The fix should be to not use ${server.addr} but 127.0.0.1 in the fetch in server_test.v:

image

@hholst80 hholst80 force-pushed the fix-tcp-tests branch 4 times, most recently from d92b92c to 8e0f7e8 Compare January 5, 2024 21:06
Test that: the listen_tcp method should not accept
.unix af.; and it indeed fails when invoked like so.

Remove $windows specific code from tcp.c.v,
that on surface inspection seems to be invalid.

Fix server_test.v to use 127.0.0.1 when connecting
to the HTTP server, as Windows does not accept
a connection to 0.0.0.0 (it works on Linux/BSD.)
@hholst80
Copy link
Contributor Author

hholst80 commented Jan 6, 2024

The failing Docker tests were really good! It showed me that the tcp_test.v IPv6 tests actually uses IPv6 now. ;-)

(The reason why I did not catch it myself first, was because I have a habit of using --network=host and there I have IPv6.)

I want to disable to IPv6 tests on environments where there is known not to exist IPv6 stack, or validate that the test fails. Any ideas on how to do that? I know this PR is getting out of hand but first I will fix it then I will chop it up into reasonable commit.

@esquerbatua
Copy link
Contributor

This also seems related with this: #20441

@JalonSolov JalonSolov added the Needs Rebase The code of the PR must be rebased over current master before it can be approved. label Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Rebase The code of the PR must be rebased over current master before it can be approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants