This is a piece of #21878, chopped off to ease review.
Peeking at the underlying socket file descriptor of Sock and checkig if it is INVALID_SOCKET is bad encapsulation and stands in the way of testing/mocking/fuzzing.
Instead use an empty unique_ptr to denote that there is no valid socket where appropriate or outright remove such checks where they are not necessary.
The default constructor Sock::Sock() is unnecessary now after recent changes, thus remove it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
If the tests are not yet complete, then that may change this PR, right?
The tests are “complete” in a sense that they are “production ready” and can be merged. “Not complete” in a sense that more tests can be added. I do not think that tweaking the tests can change this PR. The point of #21878 is to have high level code not deal with int sockets (file descriptor), but to deal with Sock instead, which can be mocked together with all the relevant operations (connect, accept, read, write, etc).
Besides adding more safety, seeing the accompanying tests would make this easier (for me anyway) to review.
The tests are in #21878, feel free to look at them :heart:
.. to have high level code not deal with int sockets (file descriptor), but to deal with Sock instead
Thanks for the additional explanation. I was wondering if you could explain this a bit more. It’s been a while since i’ve done anything with int sockets but if I remember, it was in C (not C++). Is Sock a C++ construct that makes it easier to Mock?
The tests are in #21878, feel free to look at them heart
I looked over 21878 and I see there’s some additional fuzz testing however I don’t see the Mocks that this PR enables. Am I missing the place where the Mocks are tested or is it yet to be added? Would this PR be merged before 21878?
The int sockets is the API provided by the OS. It is in C. We call it from the C++ code.
Before #21878 we would use that OS C API directly from all over the place. E.g.:
0int s = socket(...); // create the socket, see man 2 socket
1...
2connect(s, to_addr); // see man 2 connect
The problem with this is that in order to test it we have to create a real socket that really connects to the given address which is very inconvenient. It cannot be mocked.
#21878 aims to replace all such calls to the operating system with constructs like
0Sock sock = CreateSock(); // can return Sock, FuzzedSock, StaticContentsSock or another class that implements the interface
1sock->Connect(to_addr);
This way a mock Sock class can be used and the high level code avoids calling the OS socket API at all in tests or dealing/touching int sockets at all. The last piece of that is the removal of Sock::Get() and changing the high level code which checks whether the int socket stored in Sock is INVALID_SOCKET.
The problem with this is that in order to test it we have to create a real socket that really connects to the given address which is very inconvenient. It cannot be mocked.
I understand. What I am asking is if you wouldn’t mind adding the Mock tests to this PR, or create a new PR with the Mocks before merging this one?
What I am asking is if you wouldn’t mind adding the Mock tests to this PR,
I assume that by “the Mock tests” you mean the tests that are in the last 4 commits of #21878, right? If I add them to this PR then this PR would be the same as #21878.
I created this PR as a smaller piece of #21878 to ease review. If you want to peek or fully review the tests, then see the last 4 commits in #21878.
or create a new PR with the Mocks before merging this one?
Because those tests depend on removing the Sock::Get() call which is done in this PR. If the tests get merged first, this may happen: CConnman::OpenNetworkConnection() -> CConnman::ConnectNode() -> i2p::sam::Session::Connect() and later i2p::sam::Session::~Session() -> i2p::sam::Session::Disconnect() -> Sock::Get(). Calling Get() on FuzzedSock is a bit fishy, the high level code shouldn’t peek at the internal int socket and make decisions based on its value.
Are there any Mock tests that are not part of fuzz tests?
src/test/i2p_tests.cpp.
Mock in src/test/i2p_tests.cpp, or is it already merged?
Mock could be used.
FuzzedSock and StaticContentsSock are mock implementations of Sock that are already used in master.
Depending on the code being tested various methods of Sock (or a mocked implementation) are going to be called. For example, if you write an unit or fuzz test for CConnman::SocketSendData() that is only going to invoke Sock::Send().
7b6fe6761a...666c7584cf: rebase due to conflicts
re-ACK c40c25763c6acc12e54824202b7bf503d3b2ae68 per git range-diff dc905f6 666c758 c40c257
It’s good to have them in place, just noting that the Sock::IsConnected() virtual member function overrides in src/test/util/net.h and src/test/fuzz/util/net.{h.cpp} aren’t used currently or here or in the parent yet.
Sock::IsConnected() is used in src/i2p.cpp. It is important to override all methods from the mocked sock implementations because when we exercise some real code from the tests with a mocked socket, then we do not want the real IsConnected() method to be called. For example, the tests from the parent PR mock the socket with FuzzedSock and call:
0FUZZ_TARGET_INIT(connman, ...) which calls
1 Connman::OpenNetworkConnection() which calls
2 Connman::ConnectNode() which calls
3 i2p::sam::Session::Connect() which calls
4 i2p::sam::Session::CreateIfNotCreatedAlready() which calls
5 Sock::IsConnected() // must end up calling FuzzedSock::IsConnected()
git range-diff be2e748 c40c257 e536b3a
60@@ -62,12 +61,6 @@ class Sock
61 */
62 virtual Sock& operator=(Sock&& other);
63
64- /**
65- * Get the value of the contained socket.
66- * @return socket or INVALID_SOCKET if empty
67- */
68- [[nodiscard]] virtual SOCKET Get() const;
blah(this->Get())” comments as well (or replacing them with blah(m_socket)).
55@@ -56,14 +56,25 @@ BOOST_AUTO_TEST_CASE(move_constructor)
56
57 BOOST_AUTO_TEST_CASE(move_assignment)
58 {
59- const SOCKET s = CreateSocket();
60- Sock* sock1 = new Sock(s);
61- Sock* sock2 = new Sock();
62+ const SOCKET s1 = CreateSocket();
63+ const SOCKET s2 = CreateSocket();
move_assignment change to its own commit? The old code wasn’t checking that the moved-to socket got closed, so this is improving coverage, not just avoiding the default constructor?
25@@ -26,11 +26,6 @@ static constexpr auto MAX_WAIT_FOR_IO = 1s;
26 class Sock
27 {
28 public:
29- /**
30- * Default constructor, creates an empty object that does nothing when destroyed.
31- */
32- Sock();
Sock() = delete; just to be explicit and improve the error message slightly?
37@@ -38,7 +38,6 @@ BOOST_AUTO_TEST_CASE(constructor_and_destructor)
38 {
39 const SOCKET s = CreateSocket();
40 Sock* sock = new Sock(s);
41- BOOST_CHECK_EQUAL(sock->Get(), s);
Checking that you actually got the socket you wanted seems valuable (particularly for move operations). Maybe:
0class Sock {
1public:
2 /** For unit tests */
3 bool CheckSocket(SOCKET s) const { return s == m_socket; }
4...
5};
6
7// BOOST_CHECK_EQUAL(sock->Get(), s);
8 BOOST_CHECK(sock->CheckSocket(s));
operator== instead of CheckSocket.
net: remove now unnecessary Sock::Get()
Peeking at the underlying socket file descriptor of `Sock` and checkig
if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of
testing/mocking/fuzzing.
Instead use an empty unique_ptr to denote that there is no valid socket.
The socket is always valid (the underlying file descriptor is not
`INVALID_SOCKET`) when `GetBindAddress()` is called.
The socket is always valid (the underlying file descriptor is not
`INVALID_SOCKET`) when `ConnectSocketDirectly()` is called.
e536b3a67b...b44e493890: address suggestions
`Sock::Get()` was used only in `sock.{cpp,h}`. Remove it and access
`Sock::m_socket` directly.
Unit tests that used `Get()` to test for equality still verify that the
behavior is correct by using the added `operator==()`.b44e493890...384e9e3552: reword a commit message, see #26312 (review)
77 delete sock1;
78- BOOST_CHECK(!SocketIsClosed(s));
79- BOOST_CHECK_EQUAL(sock2->Get(), s);
80+ BOOST_CHECK(!SocketIsClosed(s1));
81+ BOOST_CHECK(SocketIsClosed(s2));
82+ BOOST_CHECK(*sock2 == s1);
384e9e3552 while testing, added these assertions if you think they are worthwhile (feel free to ignore)
0@@ -65,10 +66,12 @@ BOOST_AUTO_TEST_CASE(move_assignment)
1
2 BOOST_CHECK(!SocketIsClosed(s1));
3 BOOST_CHECK(!SocketIsClosed(s2));
4+ BOOST_CHECK(*sock2 != s1);
5
6 *sock2 = std::move(*sock1);
7 BOOST_CHECK(!SocketIsClosed(s1));
8 BOOST_CHECK(SocketIsClosed(s2));
9+ BOOST_CHECK(*sock2 == s1);
Use also a socket for the moved-to object and check which one is closed when.
384e9e3552...7df4508369: add one more check in the unit test: #26312 (review)
