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

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

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():

<details> <summary>rename Sock::Reset() to Sock::Close()</summary>

diff --git i/src/i2p.cpp w/src/i2p.cpp
index caff8c1e69..5897df2b0c 100644
--- i/src/i2p.cpp
+++ w/src/i2p.cpp
@@ -407,11 +407,11 @@ void Session::Disconnect()
         if (m_session_id.empty()) {
             Log("Destroying incomplete session");
         } else {
             Log("Destroying session %s", m_session_id);
         }
     }
-    m_control_sock->Reset();
+    m_control_sock->Close();
     m_session_id.clear();
 }
 } // namespace sam
 } // namespace i2p
diff --git i/src/test/fuzz/util.cpp w/src/test/fuzz/util.cpp
index c65eef9c61..111dd0cc7c 100644
--- i/src/test/fuzz/util.cpp
+++ w/src/test/fuzz/util.cpp
@@ -21,25 +21,25 @@ FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
     m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET);
 }
 
 FuzzedSock::~FuzzedSock()
 {
     // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call
-    // Sock::Reset() (not FuzzedSock::Reset()!) which will call close(m_socket).
+    // Sock::Close() (not FuzzedSock::Close()!) which will call close(m_socket).
     // Avoid closing an arbitrary file descriptor (m_socket is just a random very high number which
     // theoretically may concide with a real opened file descriptor).
-    Reset();
+    Close();
 }
 
 FuzzedSock& FuzzedSock::operator=(Sock&& other)
 {
     assert(false && "Move of Sock into FuzzedSock not allowed.");
     return *this;
 }
 
-void FuzzedSock::Reset()
+void FuzzedSock::Close()
 {
     m_socket = INVALID_SOCKET;
 }
 
 ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const
 {
diff --git i/src/test/fuzz/util.h w/src/test/fuzz/util.h
index 66d00b1767..fe589e9c06 100644
--- i/src/test/fuzz/util.h
+++ w/src/test/fuzz/util.h
@@ -52,13 +52,13 @@ public:
     explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);
 
     ~FuzzedSock() override;
 
     FuzzedSock& operator=(Sock&& other) override;
 
-    void Reset() override;
+    void Close() override;
 
     ssize_t Send(const void* data, size_t len, int flags) const override;
 
     ssize_t Recv(void* buf, size_t len, int flags) const override;
 
     int Connect(const sockaddr*, socklen_t) const override;
diff --git i/src/test/sock_tests.cpp w/src/test/sock_tests.cpp
index 4a5b25c61b..eaf1998a97 100644
--- i/src/test/sock_tests.cpp
+++ w/src/test/sock_tests.cpp
@@ -66,17 +66,17 @@ BOOST_AUTO_TEST_CASE(move_assignment)
     BOOST_CHECK(!SocketIsClosed(s));
     BOOST_CHECK_EQUAL(sock2->Get(), s);
     delete sock2;
     BOOST_CHECK(SocketIsClosed(s));
 }
 
-BOOST_AUTO_TEST_CASE(reset)
+BOOST_AUTO_TEST_CASE(close)
 {
     const SOCKET s = CreateSocket();
     Sock sock(s);
-    sock.Reset();
+    sock.Close();
     BOOST_CHECK(SocketIsClosed(s));
 }
 
 #ifndef WIN32 // Windows does not have socketpair(2).
 
 static void CreateSocketPair(int s[2])
diff --git i/src/test/util/net.h w/src/test/util/net.h
index 37d278645a..03361fa024 100644
--- i/src/test/util/net.h
+++ w/src/test/util/net.h
@@ -97,21 +97,21 @@ public:
     explicit StaticContentsSock(const std::string& contents) : m_contents{contents}, m_consumed{0}
     {
         // Just a dummy number that is not INVALID_SOCKET.
         m_socket = INVALID_SOCKET - 1;
     }
 
-    ~StaticContentsSock() override { Reset(); }
+    ~StaticContentsSock() override { Close(); }
 
     StaticContentsSock& operator=(Sock&& other) override
     {
         assert(false && "Move of Sock into MockSock not allowed.");
         return *this;
     }
 
-    void Reset() override
+    void Close() override
     {
         m_socket = INVALID_SOCKET;
     }
 
     ssize_t Send(const void*, size_t len, int) const override { return len; }
 
diff --git i/src/util/sock.cpp w/src/util/sock.cpp
index aca83d4170..ae3732e64b 100644
--- i/src/util/sock.cpp
+++ w/src/util/sock.cpp
@@ -36,25 +36,25 @@ Sock::Sock(SOCKET s) : m_socket(s) {}
 Sock::Sock(Sock&& other)
 {
     m_socket = other.m_socket;
     other.m_socket = INVALID_SOCKET;
 }
 
-Sock::~Sock() { Reset(); }
+Sock::~Sock() { Close(); }
 
 Sock& Sock::operator=(Sock&& other)
 {
-    Reset();
+    Close();
     m_socket = other.m_socket;
     other.m_socket = INVALID_SOCKET;
     return *this;
 }
 
 SOCKET Sock::Get() const { return m_socket; }
 
-void Sock::Reset() {
+void Sock::Close() {
     if (m_socket == INVALID_SOCKET) {
         return;
     }
 #ifdef WIN32
     int ret = closesocket(m_socket);
 #else
diff --git i/src/util/sock.h w/src/util/sock.h
index 71c6a49321..67f43bba7a 100644
--- i/src/util/sock.h
+++ w/src/util/sock.h
@@ -68,13 +68,13 @@ public:
      */
     [[nodiscard]] virtual SOCKET Get() const;
 
     /**
      * Close if non-empty.
      */
-    virtual void Reset();
+    virtual void Close();
 
     /**
      * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this
      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
      */
     [[nodiscard]] virtual ssize_t Send(const void* data, size_t len, int flags) const;

</details>

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

<details> <summary>remove Sock::Reset()</summary>

diff --git i/src/i2p.cpp w/src/i2p.cpp
index caff8c1e69..8611984555 100644
--- i/src/i2p.cpp
+++ w/src/i2p.cpp
@@ -407,11 +407,11 @@ void Session::Disconnect()
         if (m_session_id.empty()) {
             Log("Destroying incomplete session");
         } else {
             Log("Destroying session %s", m_session_id);
         }
     }
-    m_control_sock->Reset();
+    m_control_sock = std::make_unique<Sock>(INVALID_SOCKET);
     m_session_id.clear();
 }
 } // namespace sam
 } // namespace i2p
diff --git i/src/test/fuzz/util.cpp w/src/test/fuzz/util.cpp
index c65eef9c61..8f5e771e37 100644
--- i/src/test/fuzz/util.cpp
+++ w/src/test/fuzz/util.cpp
@@ -21,29 +21,24 @@ FuzzedSock::FuzzedSock(FuzzedDataProvider& fuzzed_data_provider)
     m_socket = fuzzed_data_provider.ConsumeIntegralInRange<SOCKET>(INVALID_SOCKET - 1, INVALID_SOCKET);
 }
 
 FuzzedSock::~FuzzedSock()
 {
     // Sock::~Sock() will be called after FuzzedSock::~FuzzedSock() and it will call
-    // Sock::Reset() (not FuzzedSock::Reset()!) which will call close(m_socket).
+    // close(m_socket) if m_socket is not INVALID_SOCKET.
     // Avoid closing an arbitrary file descriptor (m_socket is just a random very high number which
     // theoretically may concide with a real opened file descriptor).
-    Reset();
+    m_socket = INVALID_SOCKET;
 }
 
 FuzzedSock& FuzzedSock::operator=(Sock&& other)
 {
     assert(false && "Move of Sock into FuzzedSock not allowed.");
     return *this;
 }
 
-void FuzzedSock::Reset()
-{
-    m_socket = INVALID_SOCKET;
-}
-
 ssize_t FuzzedSock::Send(const void* data, size_t len, int flags) const
 {
     constexpr std::array send_errnos{
         EACCES,
         EAGAIN,
         EALREADY,
diff --git i/src/test/fuzz/util.h w/src/test/fuzz/util.h
index 66d00b1767..0819d326fd 100644
--- i/src/test/fuzz/util.h
+++ w/src/test/fuzz/util.h
@@ -52,14 +52,12 @@ public:
     explicit FuzzedSock(FuzzedDataProvider& fuzzed_data_provider);
 
     ~FuzzedSock() override;
 
     FuzzedSock& operator=(Sock&& other) override;
 
-    void Reset() override;
-
     ssize_t Send(const void* data, size_t len, int flags) const override;
 
     ssize_t Recv(void* buf, size_t len, int flags) const override;
 
     int Connect(const sockaddr*, socklen_t) const override;
 
diff --git i/src/test/sock_tests.cpp w/src/test/sock_tests.cpp
index 4a5b25c61b..01a402833d 100644
--- i/src/test/sock_tests.cpp
+++ w/src/test/sock_tests.cpp
@@ -66,20 +66,12 @@ BOOST_AUTO_TEST_CASE(move_assignment)
     BOOST_CHECK(!SocketIsClosed(s));
     BOOST_CHECK_EQUAL(sock2->Get(), s);
     delete sock2;
     BOOST_CHECK(SocketIsClosed(s));
 }
 
-BOOST_AUTO_TEST_CASE(reset)
-{
-    const SOCKET s = CreateSocket();
-    Sock sock(s);
-    sock.Reset();
-    BOOST_CHECK(SocketIsClosed(s));
-}
-
 #ifndef WIN32 // Windows does not have socketpair(2).
 
 static void CreateSocketPair(int s[2])
 {
     BOOST_REQUIRE_EQUAL(socketpair(AF_UNIX, SOCK_STREAM, 0, s), 0);
 }
diff --git i/src/test/util/net.h w/src/test/util/net.h
index 37d278645a..edb45d7c8e 100644
--- i/src/test/util/net.h
+++ w/src/test/util/net.h
@@ -97,25 +97,20 @@ public:
     explicit StaticContentsSock(const std::string& contents) : m_contents{contents}, m_consumed{0}
     {
         // Just a dummy number that is not INVALID_SOCKET.
         m_socket = INVALID_SOCKET - 1;
     }
 
-    ~StaticContentsSock() override { Reset(); }
+    ~StaticContentsSock() override { m_socket = INVALID_SOCKET; }
 
     StaticContentsSock& operator=(Sock&& other) override
     {
         assert(false && "Move of Sock into MockSock not allowed.");
         return *this;
     }
 
-    void Reset() override
-    {
-        m_socket = INVALID_SOCKET;
-    }
-
     ssize_t Send(const void*, size_t len, int) const override { return len; }
 
     ssize_t Recv(void* buf, size_t len, int flags) const override
     {
         const size_t consume_bytes{std::min(len, m_contents.size() - m_consumed)};
         std::memcpy(buf, m_contents.data() + m_consumed, consume_bytes);
diff --git i/src/util/sock.cpp w/src/util/sock.cpp
index aca83d4170..bcefa2a322 100644
--- i/src/util/sock.cpp
+++ w/src/util/sock.cpp
@@ -36,39 +36,34 @@ Sock::Sock(SOCKET s) : m_socket(s) {}
 Sock::Sock(Sock&& other)
 {
     m_socket = other.m_socket;
     other.m_socket = INVALID_SOCKET;
 }
 
-Sock::~Sock() { Reset(); }
+Sock::~Sock() { *this = Sock{INVALID_SOCKET}; }
 
 Sock& Sock::operator=(Sock&& other)
 {
-    Reset();
+    if (m_socket != INVALID_SOCKET) {
+#ifdef WIN32
+        int ret = closesocket(m_socket);
+#else
+        int ret = close(m_socket);
+#endif
+        if (ret) {
+            LogPrintf(
+                "Error closing socket %d: %s\n", m_socket, NetworkErrorString(WSAGetLastError()));
+        }
+    }
     m_socket = other.m_socket;
     other.m_socket = INVALID_SOCKET;
     return *this;
 }
 
 SOCKET Sock::Get() const { return m_socket; }
 
-void Sock::Reset() {
-    if (m_socket == INVALID_SOCKET) {
-        return;
-    }
-#ifdef WIN32
-    int ret = closesocket(m_socket);
-#else
-    int ret = close(m_socket);
-#endif
-    if (ret) {
-        LogPrintf("Error closing socket %d: %s\n", m_socket, NetworkErrorString(WSAGetLastError()));
-    }
-    m_socket = INVALID_SOCKET;
-}
-
 ssize_t Sock::Send(const void* data, size_t len, int flags) const
 {
     return send(m_socket, static_cast<const char*>(data), len, flags);
 }
 
 ssize_t Sock::Recv(void* buf, size_t len, int flags) const
diff --git i/src/util/sock.h w/src/util/sock.h
index 71c6a49321..1593a15663 100644
--- i/src/util/sock.h
+++ w/src/util/sock.h
@@ -65,17 +65,12 @@ public:
     /**
      * Get the value of the contained socket.
      * [@return](/bitcoin-bitcoin/contributor/return/) socket or INVALID_SOCKET if empty
      */
     [[nodiscard]] virtual SOCKET Get() const;
 
-    /**
-     * Close if non-empty.
-     */
-    virtual void Reset();
-
     /**
      * send(2) wrapper. Equivalent to `send(this->Get(), data, len, flags);`. Code that uses this
      * wrapper can be unit tested if this method is overridden by a mock Sock implementation.
      */
     [[nodiscard]] virtual ssize_t Send(const void* data, size_t len, int flags) const;
 

</details>

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

I like deleting code.

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)

Removed!

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?).

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.

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

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.

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.

Great, so you already thought of that, thanks!