refactor: make setsockopt() and SetSocketNoDelay() mockable/testable

vasild commented at 3:11 pm on February 16, 2022: member

This is a piece of #21878, chopped off to ease review.

Add a virtual (thus mockable) method Sock::SetSockOpt() that wraps the system setsockopt().

Convert the standalone SetSocketNoDelay() function to a virtual (thus mockable) method Sock::SetNoDelay().

This will help avoid syscalls during testing and to mock them to return whatever is suitable for the tests.

DrahtBot added the label P2P on Feb 16, 2022

DrahtBot added the label Refactoring on Feb 16, 2022

DrahtBot added the label Utils/log/libs on Feb 16, 2022

DrahtBot commented at 8:10 am on February 17, 2022: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24185 (refactor: only use explicit reinterpret/const casts, not implicit by PastaPastaPasta)
#21878 (Make all networking code mockable by vasild)

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.

jonatack commented at 4:56 pm on March 9, 2022: member

Concept ACK

in src/util/sock.h:120 in aaf8d7ed1f outdated

114@@ -115,6 +115,22 @@ class Sock
115                                          void* opt_val,
116                                          socklen_t* opt_len) const;
117 
118+    /**
119+     * setsockopt(2) wrapper. Equivalent to
120+     * `setsockopt(this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this

jonatack commented at 10:47 pm on March 15, 2022:

03c630e

0     * `setsockopt(/*sockfd=*/this->Get(), level, opt_name, opt_val, opt_len)`. Code that uses this

(same for getsockopt just above)

vasild commented at 11:41 am on March 16, 2022:

Ignoring, on my system the parameter is named s: int setsockopt(int s, ...

jonatack commented at 0:08 am on March 17, 2022:

Thanks, makes sense

in src/util/sock.h:126 in aaf8d7ed1f outdated

121+     * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
122+     */
123+    [[nodiscard]] virtual int SetSockOpt(int level,
124+                                         int opt_name,
125+                                         const void* opt_val,
126+                                         socklen_t opt_len) const;

jonatack commented at 10:50 pm on March 15, 2022:

03c630e Placing all the SetSockOpt() declaration parameters on the same line, like in the definition, would be more consistent and easier to compare with the definition.

vasild commented at 11:43 am on March 16, 2022:

Ignoring, the line would then become too long, and GetSockOpt() is already like that, so leaving it for consistency with surrounding code. Actually clang-format formats it like this, given a line length limit:

0    [[nodiscard]] virtual int            
1    SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const;

jonatack commented at 0:14 am on March 17, 2022:

given a line length limit:

There isn’t a limit, for this project the line isn’t particularly long, and it would be annoying for all the functions with several params to be formatted this way. That said, thanks for responding.

in src/net.cpp:2411 in aaf8d7ed1f outdated

2409         int nProtLevel = PROTECTION_LEVEL_UNRESTRICTED;
2410-        setsockopt(sock->Get(), IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int));
2411+        if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_PROTECTION_LEVEL, (const char*)&nProtLevel, sizeof(int)) == SOCKET_ERROR) {
2412+            strError = strprintf(
2413+                Untranslated("Error setting IPV6_PROTECTION_LEVEL on socket: %s, continuing anyway"),
2414+                NetworkErrorString(WSAGetLastError()));

jonatack commented at 11:13 pm on March 15, 2022:

7c914fe2 These 3 error messages would be easier to read and review if they were on one line like the two pre-existing ones in the same BindListenPort() function. Also, the first added one is formatted differently from the next two added ones in this commit. Each being on one line would be simpler to write and to read.

vasild commented at 11:45 am on March 16, 2022:

Changed to be all in one line, for consistency with the pre-existent ones (even though I do not like too long lines).

jonatack commented at 0:14 am on March 17, 2022:

Thanks.

jonatack commented at 11:27 pm on March 15, 2022: member

ACK aaf8d7ed1fbcd2f1a90e4f02d50ca7d2864587d0

Some style feedback, feel free to ignore.

vasild force-pushed on Mar 16, 2022

vasild commented at 11:40 am on March 16, 2022: member

aaf8d7ed1f...3ee641f452: style changes

Invalidates ACK from @jonatack

jonatack commented at 0:16 am on March 17, 2022: member

ACK 3ee641f452dc98644246252613e7590ca5ad88f

in src/util/sock.cpp:113 in 3ee641f452 outdated

104@@ -105,6 +105,17 @@ int Sock::GetSockOpt(int level, int opt_name, void* opt_val, socklen_t* opt_len)
105     return getsockopt(m_socket, level, opt_name, static_cast<char*>(opt_val), opt_len);
106 }
107 
108+int Sock::SetSockOpt(int level, int opt_name, const void* opt_val, socklen_t opt_len) const
109+{
110+    return setsockopt(m_socket, level, opt_name, static_cast<const char*>(opt_val), opt_len);
111+}
112+
113+bool Sock::SetNoDelay() const

laanwj commented at 6:52 am on April 14, 2022:

It’s a little bit ugly imo to have both a general SetSockOpt and a specific SetNoDelay on the same interface. I think we should decide either way:

Add a general SetSockOpt . Move the SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &on, sizeof(on)) into the caller, or,
Add specific calls for SetNoDelay, ReuseAddr, V6only, etc etc.

vasild commented at 7:47 am on April 15, 2022:

Good point! There is already a general SetSockOpt(), so I used it and ditched the SetNoDelay() method.

DrahtBot added the label Needs rebase on Apr 14, 2022

net: add new method Sock::SetSockOpt() that wraps setsockopt()

This will help to increase `Sock` usage and make more code mockable.

184e56d668

net: use Sock::SetSockOpt() instead of setsockopt() d65b6c3fb9

net: use Sock::SetSockOpt() instead of standalone SetSocketNoDelay()

Since the former is mockable, this makes it easier to test higher level
code that sets the TCP_NODELAY flag.

a2c4a7acd1

vasild force-pushed on Apr 15, 2022

vasild commented at 7:49 am on April 15, 2022: member

3ee641f452...a2c4a7acd1: rebase due to conflicts and address suggestion.

Invalidates ACK from @jonatack

laanwj commented at 8:17 am on April 15, 2022: member

Code review ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6

jonatack commented at 9:25 am on April 15, 2022: member

ACK a2c4a7acd1dfb2fb7e3c9dac6b3d8c9354b2e0a6 change since last review is folding Sock::SetNoDelay() into the callers

DrahtBot removed the label Needs rebase on Apr 15, 2022

laanwj merged this on Apr 19, 2022

laanwj closed this on Apr 19, 2022

vasild deleted the branch on Apr 19, 2022

sidhujag referenced this in commit f41c649dc6 on Apr 19, 2022

DrahtBot locked this on Apr 19, 2023

refactor: make setsockopt() and SetSocketNoDelay() mockable/testable #24357

Conflicts