net: Split socket create/connect #11363

If socks negotiation fail then hSocket will not be INVALID_SOCKET which is a different behavior than before. This also indicates that there is a leak in master because hSocket is not freed?

Nice Catch! Force-pushed the missing CloseSocket()s.

diff --git a/src/netbase.cpp b/src/netbase.cpp
index 18763d2..4b72826 100644
--- a/src/netbase.cpp
+++ b/src/netbase.cpp
@@ -561,11 +561,15 @@ bool ConnectThroughProxy(const proxyType &proxy, const std::string& strDest, int
         ProxyCredentials random_auth;
         static std::atomic_int counter(0);
         random_auth.username = random_auth.password = strprintf("%i", counter++);
-        if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket))
+        if (!Socks5(strDest, (unsigned short)port, &random_auth, hSocket)) {
+            CloseSocket(hSocket);
             return false;
+        }
     } else {
-        if (!Socks5(strDest, (unsigned short)port, 0, hSocket))
+        if (!Socks5(strDest, (unsigned short)port, 0, hSocket)) {
+            CloseSocket(hSocket);
             return false;
+        }
     }
     return true;
 }

CloseSocket sets hSocket to INVALID_SOCKET so this LGTM. I wonder if you could just return CloseSocket(hSocket)?

CloseSocket's return value indicates whether the close succeeded or not, which would be true in this case. Not what we want :)

Ahhh yeah... now I win the most stupid comment award.

Hah, no worries. We'll call it even for pointing out my mistake here

You need to change the return values values of all the "return false"s in here - INVALID_SOCKET is (currently) ~0, so we will pretend the socket creation succeeded.

Thanks, don't know how I missed those.

At the risk of making you redo a bunch of work, is it possible to const-ify the socket here and make the caller do the CloseSocket instead? Shits currently a maze and it'd be nice to clean it up sooner rather than try to follow it through the libevent changes.

Yes, that makes sense. Now that we don't have chained connect functions, it's much more clear what "the caller" means. The maze was only there because we had to close at all levels to be safe.

Will do.

NOSIGPIPE handling moved to CreateSocket(addrBind)?

Yes, I think part of the goal is to get random socket flag-fiddling things out of net and into netbase, its kinda split between both right now. @theuni ?

Yep. If it's something we require for all sockets, imo it makes sense to do it in CreateSocket.

Where did this move? The only remaining reference to TCP_NODELAY I see is in SetSocketNoDelay, which is called on accepted sockets only.

Its also called in netbase.cpp in CreateSocket, so should be set for outbound and inbound connection sockets, though not the listen socket, but that makes sense.

It's set on the listen socket as well (which should pass it down to accepted sockets anyway), since BindListenPort now uses CreateSocket().