SOCKET changes (CSocket RAII etc.)

[Diapolo]

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)

[laanwj]

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]

That check could then perhaps be rewritten to?

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

[laanwj]

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

[laanwj]

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.

[Diapolo]

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

[laanwj]

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

[Diapolo]

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

[laanwj]

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]

Understood and will be changed :).

[laanwj]

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

[Diapolo]

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

[laanwj]

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.

[sipa]

I like the idea of a RAII wrapper for sockets.

[Diapolo]

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

[Diapolo]

@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

[Diapolo]

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

[Diapolo]

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.

[jgarzik]

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.

[laanwj]

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

[jgarzik]

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.

[laanwj]

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

[Diapolo]

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

[laanwj]

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

[Diapolo]

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

[Diapolo]

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

[laanwj]

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

[Diapolo]

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

[Diapolo]

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

[jgarzik]

CSocket seems to add never-used methods and flags.

[Diapolo]

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

[BitcoinPullTester]

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.

[kazcw]

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.

[Diapolo]

@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?

[kazcw]

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

vhListenSocket would need to be a vector of pointers.

[laanwj]

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.

[Diapolo]

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