SOCKET changes (CSocket RAII etc.) #4348

pull Diapolo wants to merge 2 commits into bitcoin:master from Diapolo:net_cleanup_1 changing 4 files +125 −45
  1. Diapolo commented at 2:19 PM on June 16, 2014: none
    1. commit: cleanup use of CloseSocket() -- call CloseSocket() before printing any text (ensure consistent usage in the code), exception is when using NetworkErrorString()
    2. commit: add CSocket (RAII class that provides access to a SOCKET)
  2. in src/net.cpp:None in baea8b9aa3 outdated 
     509 | @@ -510,13 +510,10 @@ CNode* ConnectNode(CAddress addrConnect, const char *pszDest)
 510 |  
 511 |  void CNode::CloseSocketDisconnect()
 512 |  {
 513 | +    LogPrint("net", "disconnecting node %s\n", addrName);
 514 | +
 515 |      fDisconnect = true;
 516 | -    if (hSocket != INVALID_SOCKET)
    laanwj commented at 2:18 PM on June 17, 2014:

    Don't remove this check! hSocket can be INVALID_SOCKET if there is currently no connection open, in which case no closing needs to be done either.

    Diapolo commented at 2:22 PM on June 17, 2014:

    That check could then perhaps be rewritten to?

    <pre> if (closesocket(hSocket) != WSAENOTSOCK) LogPrint("net", "disconnecting node %s\n", addrName); </pre>

    laanwj commented at 2:31 PM on June 17, 2014:

    That would work. But I like the previous, explicit check better. It's just more clear "close the socket when we're connected" versus "try closing and log a message if it fails".

  3. in src/net.cpp:None in baea8b9aa3 outdated 
     515 |      fDisconnect = true;
 516 | -    if (hSocket != INVALID_SOCKET)
 517 | -    {
 518 | -        LogPrint("net", "disconnecting node %s\n", addrName);
 519 | -        closesocket(hSocket);
 520 | -        hSocket = INVALID_SOCKET;
    laanwj commented at 2:19 PM on June 17, 2014:

    This hSocket = INVALID_SOCKET is used to mark the connection as closed, don't remove it.

    Diapolo commented at 2:20 PM on June 17, 2014:

    closesocket(hSocket); does exactly that...

    laanwj commented at 2:29 PM on June 17, 2014:

    WTF?!? closesocket(x) changes the value of x? That's terribly unexpected. I don't like that at all.

    Diapolo commented at 2:32 PM on June 17, 2014:

    If you like or not, that IS the case for all calls to the function currently... we use closesocket() as a wrapper for the os specific ones and added some things. What would you suggest doing? IMHO it's a lot of duplication if we want to check everytime if a socket has state INVALID_SOCKET before calling our closesocket() function, no?

    laanwj commented at 2:50 PM on June 17, 2014:

    I prefer a bit of duplication over a closesocket() call that does something else than the WIN32 API specifies.

    Diapolo commented at 2:53 PM on June 17, 2014:

    I'm convinced, but AFAIK there are only the 2 lines in CNode that check for INVALID_SOCKET before closing, is this still correct after cleaning up closesocket() (which I'll gladly do)?

    laanwj commented at 4:22 PM on June 17, 2014:

    I think the goal should be to make closesocket() emulate the WIN32 closesocket on Linux, and don't redefine it when WIN32 at all. As we don't define the function on WIN32 anymore we can rename the function from myclosesocket to closesocket.

    According to http://msdn.microsoft.com/en-us/library/windows/desktop/ms737582%28v=vs.85%29.aspx this behavior is to return WSAENOTSOCK when an invalid socket handle is passed. So we can keep the check for INVALID_SOCKET inside the function, to emulate that behavior on Linux.

    Also removie the assignment of the output parameter hSocket = INVALID_SOCKET from the function - and make sure it happens everywhere where needed outside the function.

  4. in src/net.h:None in baea8b9aa3 outdated 
     338 | @@ -340,11 +339,7 @@ class CNode
 339 |  
 340 |      ~CNode()
 341 |      {
 342 | -        if (hSocket != INVALID_SOCKET)
    laanwj commented at 2:24 PM on June 17, 2014:

    Again, keep this check. hSocket can be INVALID_SOCKET, in which case closesocket should not be called on it.

    Diapolo commented at 2:29 PM on June 17, 2014:

    See my comment above, that check IS done in closesocket(hSocket); we did a redefinition of that function and added some nice checks to it (IMHO).

    Diapolo commented at 12:54 PM on July 21, 2014:

    @laanwj As you add this check in CloseSocket() this should be fine now :).

  5. in src/net.cpp:None in baea8b9aa3 outdated 
    1628 | @@ -1632,6 +1629,7 @@ bool BindListenPort(const CService &addrBind, string& strError)
1629 |  
1630 |      if (::bind(hListenSocket, (struct sockaddr*)&sockaddr, len) == SOCKET_ERROR)
1631 |      {
1632 | +        closesocket(hListenSocket);
    laanwj commented at 2:26 PM on June 17, 2014:

    Good catch, this was a potential (one-time) socket leak.

  6. laanwj commented at 2:37 PM on June 17, 2014: member

    IMHO we should get rid of myclosesocket and the crazy construction in compat.h and make it a pure compatibility wrapper (only defined on unix), without extra functionality and checks. Redefining an existing API function with unexpected behavior in the guise of a compatibility wrapper is just too crazy.

  7. Diapolo commented at 6:42 PM on June 17, 2014: none

    @laanwj I reworked our closesocket() to only be active on Non-Windows OSes, Windows now uses the pure API call.

  8. in src/compat.h:None in 651cd771c7 outdated 
      58 | @@ -59,19 +59,17 @@ typedef u_int SOCKET;
  59 |  #define SOCKET_ERROR        -1
  60 |  #endif
  61 |  
  62 | -inline int myclosesocket(SOCKET& hSocket)
  63 | +#ifndef WIN32
  64 | +// Emulate WIN32 closesocket() behaviour for other OSes
  65 | +inline int closesocket(SOCKET& hSocket)
    laanwj commented at 1:14 PM on June 18, 2014:

    this can be just SOCKET hSocket now like a normal parameter, no need for it to be a reference anymore :)

    Diapolo commented at 1:20 PM on June 18, 2014:

    Isn't a reference better because we then use no extra copy?

    laanwj commented at 1:30 PM on June 18, 2014:

    For large types such as structures and objects that can be true (but then we'd use a const reference). For small types like integers and handles it's only overhead. On 64-bit architectures a pointer to an integer is larger than the integer itself.

    Diapolo commented at 1:09 AM on June 22, 2014:

    Understood and will be changed :).

  9. laanwj commented at 1:16 PM on June 18, 2014: member

    The hard part now will be checking all existing closesocket() calls and determining if they should be followed by hSocket = INVALID_SOCKET.

  10. Diapolo commented at 1:19 PM on June 18, 2014: none

    If you mind looking at #3461 and we can get that merged, it would remove quite a few of them ;). Also the closesocket() during network init for listen sockets should be easy, which is also true for socks5(). Not sure if there are that many left then...

  11. laanwj commented at 10:37 AM on June 21, 2014: member

    Hmm, thinking further about this, another way would be to define a CSocket class with move semantics (and a method Close(), at least, to force closing), and use that instead of passing raw file handles around. This could avoid socket leaks completely by using RAII. It's somewhat more complex and involved, but as we have to change every call site of closesocket() anyway it may be something to think about.

  12. sipa commented at 8:25 PM on June 21, 2014: member

    I like the idea of a RAII wrapper for sockets.

  13. Diapolo commented at 1:08 AM on June 22, 2014: none

    @sipa @laanwj I started implementing CSocket, can you give me some feedback and help to see if I'm on the right track please :). See: https://github.com/Diapolo/bitcoin/commit/1c5873e0ba4cc466c11e5a0dbc779d78b5662075

    Edit: The new class is currently only used for listening sockets to show the current implementation state. Edit 2: Also it seems that code is currently not working ^^... seems to be too late... a look from some experienced dev would be great anyway.

  14. Diapolo commented at 11:23 AM on June 22, 2014: none

    @laanwj @sipa Alright, I was able to fix the problems from last night. During BOOST_FOREACH processing existing sockets were copied/used and got closed by the destructor after the loop ^^. This is now fixed by a new flag fCloseOnDestruct in the CSocket class. I need some initial feedback now :).

    Commit is at: https://github.com/Diapolo/bitcoin/commit/079dab40b8e2d88786d2fd0bef9025afa5719b9d

  15. Diapolo commented at 12:58 PM on June 22, 2014: none

    I'm going for some incremental pulls that are included here, see #4388 for the first one.

  16. Diapolo commented at 7:21 AM on June 24, 2014: none

    Did anyone take a look at the RAII class yet? I don't want to continue, if it's bugged or it's implementation is not correct.

  17. jgarzik commented at 2:35 PM on June 26, 2014: contributor

    If we are going to have an RAII CSocket, we might as well add classes Connection, SocketServer, IOEngine.

    Then we can ditch boost::asio for the RPC server, and use that instead.

  18. laanwj commented at 2:48 PM on June 26, 2014: member

    @diapolo No, haven't got around to it. @jgarzik I disagree we should be spending time writing and maintaining our own asynchronous I/O library. It's not like we're innovating anything here. If we want to ditch boost::asio we should switch to something which is well-known to be robust and high-performance like libevent (or derivatives).

  19. jgarzik commented at 2:53 PM on June 26, 2014: contributor

    If we don't want our own async I/O lib, then adding RAII CSocket is moving in the wrong direction. boost::asio already has portable socket classes and much much more.

  20. laanwj commented at 3:07 PM on June 26, 2014: member

    Well - in principle I agree that using the same network library everywhere would be a good thing. But I know at least @gmaxwell doesn't like boost::asio and doesn't want to switch the P2P code to it.

    So at some point we have to make a decision about what async networking library to use. Not necessarily now.

    The point here was to get rid of the strange compat.h behavior - a compatibility wrapper should provide compatibility not add functionality. I then thought, if we're changing socket closing behavior we might as well make it RAII. But if that's a stap too far, fair enough...

  21. Diapolo commented at 8:11 AM on July 1, 2014: none

    @laanwj Thinking about always call closesocket() before printing any text (ensure consistent usage in the code)... closesocket() can fail, which would likely interact with our LogPrint()... so I guess it's a good idea to first log and close the socket afterwards... but then everywhere in our code.

  22. laanwj commented at 8:16 AM on July 1, 2014: member

    @Diapolo Well in cases where the purpose of the Logprintf is to log the error it should definitely be logged before the close, otherwise it will log the status of the close(). In other cases it doesn't matter what you do first.

  23. Diapolo commented at 8:23 AM on July 1, 2014: none

    @laanwj Right, it's only relevant, when a call to NetworkErrorString() is used for the logging.

  24. laanwj referenced this in commit ca67fc7d95 on Jul 10, 2014
  25. laanwj referenced this in commit 48e40f2492 on Jul 14, 2014
  26. Diapolo commented at 3:52 PM on July 15, 2014: none

    Did anyone look at the RAII wrapper yet or is this considered unhelpful in the end? Edit: I know this needs a rebase now...

  27. laanwj commented at 9:38 AM on July 16, 2014: member

    @Diapolo I've looked at it and it looks okay, but as @jgarzik says it may be too much if we intend to move to some other async networking library for P2P. Although it is clearly the way forward - from another async networking library we'd expect nothing less - right now it may brings subtle new bugs that have to be debugged. So I prefer the more trivial change in #4504 for now.

  28. Diapolo commented at 10:34 AM on July 16, 2014: none

    @laanwj Rebased and updated to include changes for node whitelisting. I'm not sure #4504 is an alternative, as it's "just" refactoring.

  29. laanwj referenced this in commit 43f510d37d on Jul 17, 2014
  30. cleanup use of CloseSocket()
    - call CloseSocket() before printing any text (ensure consistent
  usage in the code), exception is when using NetworkErrorString()
    56e2dc2cfe
  31. Diapolo renamed this:
    extend and cleanup use of closesocket()
    SOCKET changes (CSocket RAII etc.)
    on Jul 21, 2014
  32. Diapolo commented at 12:58 PM on July 21, 2014: none

    @laanwj I reworked and rethought this, there is no need to try to emulate WIN32 closesocket() behaviour for non-WIN32, as close() gives error EBADF - fd isn't a valid open file descriptor., which we typedef as WSAENOTSOCK in compat.h anyway ;). So they behave the same already.

  33. add CSocket (RAII class that provides access to a SOCKET) 684bec2f41
  34. jgarzik commented at 1:01 PM on July 21, 2014: contributor

    CSocket seems to add never-used methods and flags.

  35. Diapolo commented at 1:11 PM on July 21, 2014: none

    @jgarzik Right, the ones who are unused are void SetCloseOnDestruct(bool fFlag); and void SetWhitelisted(bool fFlag); and I mentioned that in a comment in the source. Also CSocket is currently ONLY used for listen sockets, because I didn't want to change the whole source, before anyone gave some feedback to my implementation.

  36. BitcoinPullTester commented at 1:31 PM on July 21, 2014: none

    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4348_684bec2f41d0963fe24cf55cd397b962a25529cf/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

  37. kazcw commented at 5:17 PM on July 21, 2014: contributor

    The move semantics seem important. Right now you can copy or assign a socket and have two sockets using the same handle; then if one goes out of scope or is Close()ed the other will have its underlying socket closed without CloseSocket setting hSocket to INVALID_SOCKET, so it still returns IsValid()==true. With cloning disallowed, fCloseOnDestruct shouldn't be necessary.

    Putting the "whitelisted" flag in what would otherwise be a general RAII socket wrapper seems odd. Why not keep the ListenSocket struct, and just change it from using a handle to a CSocket?

    Having the Get() method is no different from just making the hSocket a public member. I suggest either adding wrappers for the functions that take sockets (non-member friend functions could do the FD_SET operations), or if CSocket is to be a struct with a destructor, making it look like a struct with a destructor.

  38. Diapolo commented at 3:48 PM on July 25, 2014: none

    @kazcw I guess I need some more help to implement what you suggest about disallow cloning. Makes sense that no CSocket should have the same handle. How can this be achieved?

  39. kazcw commented at 5:12 PM on July 25, 2014: contributor

    To prevent copying, you can declare a private copy constructor with no implementation.

    vhListenSocket would need to be a vector of pointers.

  40. laanwj added the label P2P on Jul 31, 2014
  41. laanwj commented at 3:40 PM on August 4, 2014: member

    As said before, I deem the RAII changes too risky for what it would bring and am not going to merge it, sorry.

    This would make sense as part of a change that wraps all networking behavior in proper classes, but if we make such a high-impact change to networking we should consider switching to an existing high-performance async networking library instead of mucking about with our own.

    Something to consider in the future, but right now there are much more pressing things.

  42. laanwj closed this on Aug 4, 2014
  43. Diapolo commented at 11:42 AM on August 5, 2014: none

    @laanwj I created #4636 as these changes seem uncontroversial.

  44. Diapolo deleted the branch on Aug 5, 2014
  45. MathyV referenced this in commit 05ea249faa on Nov 24, 2014
  46. reddink referenced this in commit b5a0c6bf9d on May 27, 2020
  47. MarcoFalke locked this on Sep 8, 2021
Contributors
DiapololaanwjsipajgarzikBitcoinPullTesterkazcw
Labels
P2P
Linked (view graph)
#4388 incremental net cleanup 1#4390 incremental net cleanup 2#4504 Convert closesocket 'compat wrapper' to function in netbase
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 18:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me