test: Add testing of connect()-ion abort ability #2301
Conversation
There was a problem hiding this comment.
If this is a bug, please open an issue for it and "Refs" to it from this test.
If the test is know to fail on io-uring, how can we have it in our test suite? Maybe we need something like "xfail"?
But I don't understand what this test does. It appears it calls sk.shutdown() (which I assume has nothing to do with shutdown(2) :-( ), and then doesn't check anything else.
tests/unit/socket_test.cc
Outdated
| return seastar::async([&] { | ||
| bool too_late = false; | ||
| auto sk = make_socket(); | ||
| auto cf = sk.connect(ipv4_addr("10.255.255.1", 12345)).then([] (auto cs) { |
There was a problem hiding this comment.
What is supposed to happen in this address? I assume you hope it will never respond?
Maybe it's better to use a really reserved address for this? E.g., something from TEST-NET-1 (192.0.2.0/24). See https://www.rfc-editor.org/rfc/rfc5735
There was a problem hiding this comment.
What is supposed to happen in this address? I assume you hope it will never respond?
Yes. 10.0.0.0/8 is reserved range for local communications, I assume that 10.255.255.* is not routable in any ... practical environments like CI instances which seems to be true
Maybe it's better to use a really reserved address for this? E.g., something from TEST-NET-1 (192.0.2.0/24). See https://www.rfc-editor.org/rfc/rfc5735
Ah, 192.0.2.0/24 sounds much better, thanks
tests/unit/socket_test.cc
Outdated
|
|
||
| auto check = sleep(std::chrono::seconds(2)).then([&too_late] { | ||
| fmt::print("Connection must have been aborted already\n"); | ||
| too_late = true; |
There was a problem hiding this comment.
Where is it defined that connect() should time out in less than 2 seconds?
There was a problem hiding this comment.
Oh, I think I understand now - it's not about timeout at all. The code below after 0.5 seconds calls shutdown() on the connection, and then the connect() code (above) will stop and check too_late wasn't yet set. And it wasn't - it will be set only after 2 seconds.
Maybe the code could have been organized in a different order to make this clearer, but at least it looks corect.
There was a problem hiding this comment.
Yes, it's exactly as you described.
Already there -- #2302
The CI runs with aio backend which works
It is shutdown(2) for TCP sockets: virtual void shutdown() override {
if (_fd) {
try {
_fd.shutdown(SHUT_RDWR);
} catch (std::system_error& e) {
if (e.code().value() != ENOTCONN) {
throw;
}
}
}
} |
xemul
force-pushed
the
br-socket-connect-abort-test
branch
from
82a5204
to
ec4546b
Compare
|
upd:
|
Thanks, so please "Refs" to it from this PR.
This is very awkward :-( It suggests we some specific tests which check networking and some other features, we need to run them multiple times with different backends... Of course, you don't need to fix this in this PR. |
Docs say, that calling socket::shutdown() aborts any in-flight connection. This happens to be true for linux-aio backend, but not for the io-uring one. Here's the test. Signed-off-by: Pavel Emelyanov <[email protected]>
xemul
force-pushed
the
br-socket-connect-abort-test
branch
from
ec4546b
to
d2beaab
Compare
Docs say, that calling socket::shutdown() aborts any in-flight connection. This happens to be true for linux-aio backend, but not for the io-uring one. Here's the test. Signed-off-by: Pavel Emelyanov <[email protected]> Closes scylladb#2301
Docs say, that calling socket::shutdown() aborts any in-flight connection. This happens to be true for linux-aio backend, but not for the io-uring one. Here's the test. Signed-off-by: Pavel Emelyanov <[email protected]> Closes scylladb#2301
Docs say, that calling socket::shutdown() aborts any in-flight connection. This happens to be true for linux-aio backend, but not for the io-uring one. Here's the test.