Remove Sock::Release() and CloseSocket()

vasild commented at 2:44 pm on June 20, 2022: member

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

Sock::Release() is unused, thus remove it
CloseSocket() is only called from Sock::Reset(), so move the body of CloseSocket() inside Sock::Reset() and remove CloseSocket() - this helps to hide low level file descriptor sockets inside the Sock class.
Rename Sock::Reset() to Sock::Close() and make it private - to be used only in the destructor and in the Sock assignment operator. This simplifies the public API by removing one method from it.

net: remove now unused Sock::Release() 175fb2670a

net: remove CloseSocket()

Do the closing in `Sock::Reset()` and remove the standalone
`CloseSocket()`.

This reduces the exposure of low-level sockets (i.e. integer file
descriptors) outside of the `Sock` class.

e8ff3f0c52

DrahtBot added the label Utils/log/libs on Jun 20, 2022

in src/util/sock.cpp:54 in e8ff3f0c52 outdated

50@@ -51,15 +51,21 @@ Sock& Sock::operator=(Sock&& other)
51 
52 SOCKET Sock::Get() const { return m_socket; }
53 
54-SOCKET Sock::Release()
55-{
56-    const SOCKET s = m_socket;
57+void Sock::Reset() {

laanwj commented at 4:12 pm on June 20, 2022:

Reset is a strange API function for a socket. Conceptually, I like Close better (which is what it does, right?).

vasild commented at 12:57 pm on June 21, 2022:

Sock::Reset() existed before this PR. I picked up that name to mimic unique_ptr::reset(). Sock::Get() mimics unique_ptr::get(), as the Sock class initially came up as a RAII sock manager, mimicing unique_ptr. However, it has morphed since and soon the Sock::Get() method is to be removed.

Here is a patch that renames Sock::Reset() to Sock::Close():

  0diff --git i/src/i2p.cpp w/src/i2p.cpp
  1index caff8c1e69..5897df2b0c 100644
  2--- i/src/i2p.cpp
  3+++ w/src/i2p.cpp
  4@@ -407,11 +407,11 @@ void Session::Disconnect()
  5         if (m_session_id.empty()) {
  6             Log("Destroying incomplete session");
  7         } else {
  8             Log("Destroying session %s", m_session_id);
  9         }
 10     }
 11-    m_control_sock->Reset();
 12+    m_control_sock->Close();
 13     m_session_id.clear();
 14 }
 15 } // namespace sam
 16 } // namespace i2p
 17diff --git i/src/test/fuzz/util.cpp w/src/test/fuzz/util.cpp
 18index c65eef9c61..111dd0cc7c 100644
 19--- i/src/test/fuzz/util.cpp
 20+++ w/src/test/fuzz/util.cpp
 21@@ -21,25 +21,25 @@ FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
 22     m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET);
 23 }
 24 
 25 FuzzedSock::~FuzzedSock()
 26 {
 27     // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call
 28-    // Sock::Reset() (not FuzzedSock::Reset()!) which will call close(m_socket).
 29+    // Sock::Close() (not FuzzedSock::Close()!) which will call close(m_socket).
 30     // Avoid closing an arbitrary file descriptor (m_socket is just a random very high number which
 31     // theoretically may concide with a real opened file descriptor).
 32-    Reset();
 33+    Close();
 34 }
 35 
 36 FuzzedSock& FuzzedSock::operator=(Sock&& other)
 37 {
 38     assert(false && "Move of Sock into FuzzedSock not allowed.");
 39     return *this;
 40 }
 41 
 42-void FuzzedSock::Reset()
 43+void FuzzedSock::Close()
 44 {
 45     m_socket = INVALID_SOCKET;
 46 }
 47 
 48 ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const
 49 {
 50diff --git i/src/test/fuzz/util.h w/src/test/fuzz/util.h
 51index 66d00b1767..fe589e9c06 100644
 52--- i/src/test/fuzz/util.h
 53+++ w/src/test/fuzz/util.h
 54@@ -52,13 +52,13 @@ public:
 55     explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);
 56 
 57     ~FuzzedSock() override;
 58 
 59     FuzzedSock& operator=(Sock&& other) override;
 60 
 61-    void Reset() override;
 62+    void Close() override;
 63 
 64     ssize_t Send(const void* data, size_t len, int flags) const override;
 65 
 66     ssize_t Recv(void* buf, size_t len, int flags) const override;
 67 
 68     int Connect(const sockaddr*, socklen_t) const override;
 69diff --git i/src/test/sock_tests.cpp w/src/test/sock_tests.cpp
 70index 4a5b25c61b..eaf1998a97 100644
 71--- i/src/test/sock_tests.cpp
 72+++ w/src/test/sock_tests.cpp
 73@@ -66,17 +66,17 @@ BOOST_AUTO_TEST_CASE(move_assignment)
 74     BOOST_CHECK(!SocketIsClosed(s));
 75     BOOST_CHECK_EQUAL(sock2->Get(), s);
 76     delete sock2;
 77     BOOST_CHECK(SocketIsClosed(s));
 78 }
 79 
 80-BOOST_AUTO_TEST_CASE(reset)
 81+BOOST_AUTO_TEST_CASE(close)
 82 {
 83     const SOCKET s = CreateSocket();
 84     Sock sock(s);
 85-    sock.Reset();
 86+    sock.Close();
 87     BOOST_CHECK(SocketIsClosed(s));
 88 }
 89 
 90 #ifndef WIN32 // Windows does not have socketpair(2).
 91 
 92 static void CreateSocketPair(int s[2])
 93diff --git i/src/test/util/net.h w/src/test/util/net.h
 94index 37d278645a..03361fa024 100644
 95--- i/src/test/util/net.h
 96+++ w/src/test/util/net.h
 97@@ -97,21 +97,21 @@ public:
 98     explicit StaticContentsSock(const std::string& contents) : m_contents{contents}, m_consumed{0}
 99     {
100         // Just a dummy number that is not INVALID_SOCKET.
101         m_socket = INVALID_SOCKET - 1;
102     }
103 
104-    ~StaticContentsSock() override { Reset(); }
105+    ~StaticContentsSock() override { Close(); }
106 
107     StaticContentsSock& operator=(Sock&& other) override
108     {
109         assert(false && "Move of Sock into MockSock not allowed.");
110         return *this;
111     }
112 
113-    void Reset() override
114+    void Close() override
115     {
116         m_socket = INVALID_SOCKET;
117     }
118 
119     ssize_t Send(const void*, size_t len, int) const override { return len; }
120 
121diff --git i/src/util/sock.cpp w/src/util/sock.cpp
122index aca83d4170..ae3732e64b 100644
123--- i/src/util/sock.cpp
124+++ w/src/util/sock.cpp
125@@ -36,25 +36,25 @@ Sock::Sock(SOCKET s) : m_socket(s) {}
126 Sock::Sock(Sock&& other)
127 {
128     m_socket = other.m_socket;
129     other.m_socket = INVALID_SOCKET;
130 }
131 
132-Sock::~Sock() { Reset(); }
133+Sock::~Sock() { Close(); }
134 
135 Sock& Sock::operator=(Sock&& other)
136 {
137-    Reset();
138+    Close();
139     m_socket = other.m_socket;
140     other.m_socket = INVALID_SOCKET;
141     return *this;
142 }
143 
144 SOCKET Sock::Get() const { return m_socket; }
145 
146-void Sock::Reset() {
147+void Sock::Close() {
148     if (m_socket == INVALID_SOCKET) {
149         return;
150     }
151 #ifdef WIN32
152     int ret = closesocket(m_socket);
153 #else
154diff --git i/src/util/sock.h w/src/util/sock.h
155index 71c6a49321..67f43bba7a 100644
156--- i/src/util/sock.h
157+++ w/src/util/sock.h
158@@ -68,13 +68,13 @@ public:
159      */
160     [[nodiscard]] virtual SOCKET Get() const;
161 
162     /**
163      * Close if non-empty.
164      */
165-    virtual void Reset();
166+    virtual void Close();
167 
168     /**
169      * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this
170      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
171      */
172     [[nodiscard]] virtual ssize_t Send(const void* data, size_t len, int flags) const;

Or, maybe even better, remove the Sock::Reset() method from the Sock API, it is not used much:

  0diff --git i/src/i2p.cpp w/src/i2p.cpp
  1index caff8c1e69..8611984555 100644
  2--- i/src/i2p.cpp
  3+++ w/src/i2p.cpp
  4@@ -407,11 +407,11 @@ void Session::Disconnect()
  5         if (m_session_id.empty()) {
  6             Log("Destroying incomplete session");
  7         } else {
  8             Log("Destroying session %s", m_session_id);
  9         }
 10     }
 11-    m_control_sock->Reset();
 12+    m_control_sock = std::make_unique<Sock>(INVALID_SOCKET);
 13     m_session_id.clear();
 14 }
 15 } // namespace sam
 16 } // namespace i2p
 17diff --git i/src/test/fuzz/util.cpp w/src/test/fuzz/util.cpp
 18index c65eef9c61..8f5e771e37 100644
 19--- i/src/test/fuzz/util.cpp
 20+++ w/src/test/fuzz/util.cpp
 21@@ -21,29 +21,24 @@ FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
 22     m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET);
 23 }
 24 
 25 FuzzedSock::~FuzzedSock()
 26 {
 27     // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call
 28-    // Sock::Reset() (not FuzzedSock::Reset()!) which will call close(m_socket).
 29+    // close(m_socket) if m_socket is not INVALID_SOCKET.
 30     // Avoid closing an arbitrary file descriptor (m_socket is just a random very high number which
 31     // theoretically may concide with a real opened file descriptor).
 32-    Reset();
 33+    m_socket = INVALID_SOCKET;
 34 }
 35 
 36 FuzzedSock& FuzzedSock::operator=(Sock&& other)
 37 {
 38     assert(false && "Move of Sock into FuzzedSock not allowed.");
 39     return *this;
 40 }
 41 
 42-void FuzzedSock::Reset()
 43-{
 44-    m_socket = INVALID_SOCKET;
 45-}
 46-
 47 ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const
 48 {
 49     constexpr std::array send_errnos{
 50         EACCES,
 51         EAGAIN,
 52         EALREADY,
 53diff --git i/src/test/fuzz/util.h w/src/test/fuzz/util.h
 54index 66d00b1767..0819d326fd 100644
 55--- i/src/test/fuzz/util.h
 56+++ w/src/test/fuzz/util.h
 57@@ -52,14 +52,12 @@ public:
 58     explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);
 59 
 60     ~FuzzedSock() override;
 61 
 62     FuzzedSock& operator=(Sock&& other) override;
 63 
 64-    void Reset() override;
 65-
 66     ssize_t Send(const void* data, size_t len, int flags) const override;
 67 
 68     ssize_t Recv(void* buf, size_t len, int flags) const override;
 69 
 70     int Connect(const sockaddr*, socklen_t) const override;
 71 
 72diff --git i/src/test/sock_tests.cpp w/src/test/sock_tests.cpp
 73index 4a5b25c61b..01a402833d 100644
 74--- i/src/test/sock_tests.cpp
 75+++ w/src/test/sock_tests.cpp
 76@@ -66,20 +66,12 @@ BOOST_AUTO_TEST_CASE(move_assignment)
 77     BOOST_CHECK(!SocketIsClosed(s));
 78     BOOST_CHECK_EQUAL(sock2->Get(), s);
 79     delete sock2;
 80     BOOST_CHECK(SocketIsClosed(s));
 81 }
 82 
 83-BOOST_AUTO_TEST_CASE(reset)
 84-{
 85-    const SOCKET s = CreateSocket();
 86-    Sock sock(s);
 87-    sock.Reset();
 88-    BOOST_CHECK(SocketIsClosed(s));
 89-}
 90-
 91 #ifndef WIN32 // Windows does not have socketpair(2).
 92 
 93 static void CreateSocketPair(int s[2])
 94 {
 95     BOOST_REQUIRE_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, s), 0);
 96 }
 97diff --git i/src/test/util/net.h w/src/test/util/net.h
 98index 37d278645a..edb45d7c8e 100644
 99--- i/src/test/util/net.h
100+++ w/src/test/util/net.h
101@@ -97,25 +97,20 @@ public:
102     explicit StaticContentsSock(const std::string& contents) : m_contents{contents}, m_consumed{0}
103     {
104         // Just a dummy number that is not INVALID_SOCKET.
105         m_socket = INVALID_SOCKET - 1;
106     }
107 
108-    ~StaticContentsSock() override { Reset(); }
109+    ~StaticContentsSock() override { m_socket = INVALID_SOCKET; }
110 
111     StaticContentsSock& operator=(Sock&& other) override
112     {
113         assert(false && "Move of Sock into MockSock not allowed.");
114         return *this;
115     }
116 
117-    void Reset() override
118-    {
119-        m_socket = INVALID_SOCKET;
120-    }
121-
122     ssize_t Send(const void*, size_t len, int) const override { return len; }
123 
124     ssize_t Recv(void* buf, size_t len, int flags) const override
125     {
126         const size_t consume_bytes{std::min(len, m_contents.size() - m_consumed)};
127         std::memcpy(buf, m_contents.data() + m_consumed, consume_bytes);
128diff --git i/src/util/sock.cpp w/src/util/sock.cpp
129index aca83d4170..bcefa2a322 100644
130--- i/src/util/sock.cpp
131+++ w/src/util/sock.cpp
132@@ -36,39 +36,34 @@ Sock::Sock(SOCKET s) : m_socket(s) {}
133 Sock::Sock(Sock&& other)
134 {
135     m_socket = other.m_socket;
136     other.m_socket = INVALID_SOCKET;
137 }
138 
139-Sock::~Sock() { Reset(); }
140+Sock::~Sock() { *this = Sock{INVALID_SOCKET}; }
141 
142 Sock& Sock::operator=(Sock&& other)
143 {
144-    Reset();
145+    if (m_socket != INVALID_SOCKET) {
146+#ifdef WIN32
147+        int ret = closesocket(m_socket);
148+#else
149+        int ret = close(m_socket);
150+#endif
151+        if (ret) {
152+            LogPrintf(
153+                "Error closing socket %d: %s\n", m_socket, NetworkErrorString(WSAGetLastError()));
154+        }
155+    }
156     m_socket = other.m_socket;
157     other.m_socket = INVALID_SOCKET;
158     return *this;
159 }
160 
161 SOCKET Sock::Get() const { return m_socket; }
162 
163-void Sock::Reset() {
164-    if (m_socket == INVALID_SOCKET) {
165-        return;
166-    }
167-#ifdef WIN32
168-    int ret = closesocket(m_socket);
169-#else
170-    int ret = close(m_socket);
171-#endif
172-    if (ret) {
173-        LogPrintf("Error closing socket %d: %s\n", m_socket, NetworkErrorString(WSAGetLastError()));
174-    }
175-    m_socket = INVALID_SOCKET;
176-}
177-
178 ssize_t Sock::Send(const void* data, size_t len, int flags) const
179 {
180     return send(m_socket, static_cast<const char*>(data), len, flags);
181 }
182 
183 ssize_t Sock::Recv(void* buf, size_t len, int flags) const
184diff --git i/src/util/sock.h w/src/util/sock.h
185index 71c6a49321..1593a15663 100644
186--- i/src/util/sock.h
187+++ w/src/util/sock.h
188@@ -65,17 +65,12 @@ public:
189     /**
190      * Get the value of the contained socket.
191      * [@return](/bitcoin-bitcoin/contributor/return/) socket or INVALID_SOCKET if empty
192      */
193     [[nodiscard]] virtual SOCKET Get() const;
194 
195-    /**
196-     * Close if non-empty.
197-     */
198-    virtual void Reset();
199-
200     /**
201      * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this
202      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
203      */
204     [[nodiscard]] virtual ssize_t Send(const void* data, size_t len, int flags) const;
205

What to do? Rename, remove, leave it as is?

I like deleting code.

laanwj commented at 1:03 pm on June 21, 2022:

If you can remove it that’d be even better. If it isn’t a lot of impact on the code. I mean it’s a RAII wrapper, I’d assume normally it’d only gets closed on destrucction.

(to be clear, the rename patch is fine with me too! My preference is Remove > Rename > Leave as is)

vasild commented at 1:26 pm on June 21, 2022:

Removed!

DrahtBot commented at 11:57 pm on June 20, 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:

#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.

vasild commented at 1:27 pm on June 21, 2022: member

e8ff3f0c52..e0537dada6: remove Sock::Reset()

vasild renamed this:
~~Remove Sock::Release() and CloseSocket()~~
Remove Sock::Release(), Sock::Reset() and CloseSocket()
on Jun 21, 2022

laanwj commented at 8:40 pm on June 21, 2022: member

Concept ACK! Nice cleanup now!

net: rename Sock::Reset() to Sock::Close() and make it private

Outside of `Sock`, `Sock::Reset()` was used in just one place (in
`i2p.cpp`) which can use the assignment operator instead.

This simplifies the public `Sock` API by having one method less.

a724c39606

in src/util/sock.cpp:42 in e0537dada6 outdated

38@@ -39,27 +39,28 @@ Sock::Sock(Sock&& other)
39     other.m_socket = INVALID_SOCKET;
40 }
41 
42-Sock::~Sock() { Reset(); }
43+Sock::~Sock() { *this = Sock{INVALID_SOCKET}; }

laanwj commented at 8:42 pm on June 21, 2022:

Assigning *this in the destructor looks a bit strange to me. Let’s be sure that this isn’t undefined behavior or footguns of some kind (it can’t ever trigger a recursive constructor call, can it?).

vasild commented at 7:26 am on June 22, 2022:

You are right! The problem is not the assignment operator, but constructing a temporary Sock in the destructor which, at the end of the destructor, gets destructed… :bomb: :fire:

I renamed the Reset() method to Close() and made it private, solely for the purpose of avoiding duplicating code in the destructor and in the assignment operator. But still removed it from the public API.

laanwj commented at 8:12 am on June 22, 2022:

Oops! Yes, sounds like a more straightforwardly safe solution to me. Thanks.

vasild force-pushed on Jun 22, 2022

vasild commented at 7:29 am on June 22, 2022: member

e0537dada6...a724c39606: make Close() private and call it from both the destructor and the assignment operator.

vasild renamed this:
~~Remove Sock::Release(), Sock::Reset() and CloseSocket()~~
Remove Sock::Release() and CloseSocket()
on Jun 22, 2022

laanwj commented at 8:14 am on June 22, 2022: member

Code review ACK a724c39606273dfe4c6f9887ef8b77d0a98f1b34

in src/i2p.cpp:413 in a724c39606

409@@ -410,7 +410,7 @@ void Session::Disconnect()
410             Log("Destroying session %s", m_session_id);
411         }
412     }
413-    m_control_sock->Reset();
414+    m_control_sock = std::make_unique<Sock>(INVALID_SOCKET);

laanwj commented at 8:16 am on June 22, 2022:

Last nit: could this be m_control_sock.release(), or does the smart pointer always need to point to something?

Edit: I think that requires more changes, seems fine to keep it for a follow-up.

vasild commented at 8:42 am on June 22, 2022:

In the current code the smart pointer always has to point to something because we unconditionally dereference it in a few places.

However, that change is coming in #21878, from commit i2p: avoid using Sock::Get() for checking for a valid socket - it will use an empty unique_ptr to denote that there is no valid socket and together with a few other commits will remove the Sock::Get() method so that the value of the underlying socket file descriptor will be completely hidden inside the Sock class.

laanwj commented at 8:57 am on June 22, 2022:

Great, so you already thought of that, thanks!

laanwj merged this on Jun 22, 2022

laanwj closed this on Jun 22, 2022

vasild deleted the branch on Jun 22, 2022

sidhujag referenced this in commit e658cf5845 on Jun 22, 2022

DrahtBot locked this on Jun 22, 2023

Remove Sock::Release() and CloseSocket() #25428

Conflicts