vlib: fix tcp_test.v #20389
vlib: fix tcp_test.v #20389
Conversation
hholst80
force-pushed
the
fix-tcp-tests
branch
2 times, most recently
from
de5f494
to
46adaac
Compare
|
Panics on Windows... maybe part of why it was marked flaky... |
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
force-pushed
the
fix-tcp-tests
branch
from
46adaac
to
1234b88
Compare
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. |
vlib/net/tcp_test.v
Outdated
| mut conn := net.dial_tcp_with_bind('vlang.io:80', '127.0.0.1:0')! | ||
| conn.close()! |
There was a problem hiding this comment.
How could this ever have worked? test_bind() is not declared with ! so I thought conn.close()! would be illegal (nothing is returned)?
There was a problem hiding this comment.
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() ? {.
There was a problem hiding this comment.
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 🤷🏻♂️ .
|
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
|
hholst80
force-pushed
the
fix-tcp-tests
branch
4 times, most recently
from
d92b92c
to
8e0f7e8
Compare
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
force-pushed
the
fix-tcp-tests
branch
from
8e0f7e8
to
3dd955e
Compare
|
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. |
|
This also seems related with this: #20441 |
JalonSolov
added
the
Needs Rebase
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).