WIP: switch to libevent for node socket handling #11227

theuni commented at 9:13 PM on September 3, 2017: member

Not yet ready for review. This can be considered a staging area. Chunks of this will be PR'd until only the actual libevent switch-over is ready, at which point this PR should be ready for review.

These changes remove our old select() loop for socket handling in favor of libevent, which uses epoll/kqueue/etc. as a back-end. In addition to being faster and more efficient, this allows us to drop some annoying restrictions, namely that select can only handle 1024 sockets in practice on most systems.

Note that this does not yet make the switch to libevent for outgoing connections, that work is happening in parallel, and will be easier to merge after this.

Also, for any reviewers, several of these commits would individually introduce some regressions or slow-downs, but they've been split up in order to clarify why some of the changes are being made.

Depends on:

Todo:

Add a ton of documentation
RAII the libevent structures
Add some tests where possible

theuni force-pushed on Sep 3, 2017

theuni commented at 12:16 AM on September 4, 2017: member

I believe that the proxy_test failure is a false-positive.

Looks like the issue is that disconnection happens so quickly now after an error that the testnode fails.

theuni force-pushed on Sep 4, 2017

fanquake added the label P2P on Sep 4, 2017

fanquake added the label Refactoring on Sep 4, 2017

in src/net.cpp:457 in 82625c5a40 outdated

 453 | @@ -454,7 +454,7 @@ void CConnman::DumpBanlist()
 454 |  
 455 |  void CNode::CloseSocketDisconnect()
 456 |  {
 457 | -    fDisconnect = true;
 458 | +    Disconnect();

sipa commented at 4:07 AM on September 4, 2017:

It looks like this could be a scripted diff, s/fDisconnect = true/Disconnect()/g.

theuni commented at 10:34 AM on September 4, 2017:

Probably so, will give it a shot.

in src/net.cpp:875 in f7ff0a794f outdated

 913 | @@ -914,12 +914,7 @@ size_t CConnman::SocketSendData(CNode *pnode) const
 914 |      while (it != pnode->vSendMsg.end()) {
 915 |          const auto &data = *it;
 916 |          assert(data.size() > pnode->nSendOffset);
 917 | -        int nBytes = 0;
 918 | -        {
 919 | -            if (pnode->hSocket == INVALID_SOCKET)

sipa commented at 6:20 AM on September 4, 2017:

This test is no longer needed?

theuni commented at 10:37 AM on September 4, 2017:

I believe all of these are gone by the end of the change set, the removal may make more sense in an earlier commit, though.

in src/net.cpp:1400 in fa64f69c37 outdated

1306 | -    {
1307 | -        LogPrintf("connection from %s dropped: non-selectable socket\n", addr.ToString());
1308 | -        CloseSocket(hSocket);
1309 | -        return;
1310 | +    const char* method = event_base_get_method(m_event_base);
1311 | +    if (!method || (strncmp(method, "select", 6) == 0)) {

sipa commented at 6:37 AM on September 4, 2017:

Heh, I'd have hoped that this restriction wouldn't be needed anymore with libevent.

theuni commented at 10:38 AM on September 4, 2017:

It's only needed if select ends up being used, which should be very unlikely on most platforms. I'll add a comment here, and print something to the log so that it's clear which back-end is being used.

laanwj commented at 9:57 PM on September 5, 2017:

We could just bail out if select is being used. But not sure what platforms would be affected.

in src/net.cpp:2491 in fa64f69c37 outdated

2369 | @@ -2397,6 +2370,9 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2370 |          LOCK(m_cs_interrupt_event);
2371 |          m_interrupt_event = event_new(m_event_base, -1, 0, [](evutil_socket_t, short, void* data) {static_cast<CConnman*>(data)->InterruptEvents();}, (this));
2372 |      }
2373 | +    m_event_timeout = timeval{60, 0};
2374 | +    const timeval* common_timeout = event_base_init_common_timeout(m_event_base, &m_event_timeout);
2375 | +    memcpy(&m_event_timeout, common_timeout, sizeof(m_event_timeout));

sipa commented at 7:16 AM on September 4, 2017:

The documentation on event_base_init_common_timeout says "To do this, call this function with the common duration. It will return a pointer to a different, opaque timeout value. (Don't depend on its actual contents!)".

I'm not sure whether that means you need to use the exact pointer returned, or just a copy of its value (as this code does).

theuni commented at 10:41 AM on September 4, 2017:

Good eye! I wasn't sure how to store this properly either. This usage is taken from the official documentation. I'll add a comment about its sanity and a link to the docs.

in src/net.cpp:526 in fa64f69c37 outdated

 470 | @@ -456,7 +471,14 @@ void CConnman::DumpBanlist()
 471 |  
 472 |  void CNode::CloseSocketDisconnect()
 473 |  {
 474 | -    Disconnect();
 475 | +    if (m_write_event) {

sipa commented at 7:22 AM on September 4, 2017:

I would prefer it if you could have some RAII wrappers around the libevent structures before adding all this.

theuni commented at 10:37 AM on September 4, 2017:

Agreed, see PR description :)

laanwj commented at 9:56 PM on September 5, 2017: member

Concept ACK, woohoo!

in src/net.cpp:918 in 7274a18dc3 outdated

 913 | +                pnode->fPauseRecv = true;
 914 | +                event_del(pnode->m_read_event);
 915 | +            }
 916 |  
 917 | +        }
 918 | +        WakeMessageHandler();

jimpo commented at 11:00 PM on September 5, 2017:

Should this be moved into the above if block? (It used to be inside the if (notify) statement)

theuni commented at 12:02 AM on September 6, 2017:

Yes, thank you.

in src/net.cpp:1321 in 0d08cb3d89 outdated

1149 | @@ -1150,7 +1150,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
1150 |          }
1151 |      }
1152 |  
1153 | -    bool whitelisted = hListenSocket.whitelisted || IsWhitelistedRange(addr);
1154 | +    whitelisted = whitelisted || IsWhitelistedRange(addr);

sipa commented at 11:18 PM on September 5, 2017:

||=

I don't think such an operator exists in C++.

jimpo commented at 11:21 PM on September 5, 2017:

Oops, you're right. Should have checked.

theuni commented at 12:04 AM on September 6, 2017:

I can say for certain that the operator doesn't exist because I tried it first :)

laanwj commented at 6:12 PM on September 6, 2017:

Hehe I also remember it doesn't exist - I looked it up a long time ago and IIRC the rationale is that's because |= is the same when using only booleans. In any case this is ok as it is.

theuni force-pushed on Sep 12, 2017

theuni force-pushed on Sep 19, 2017

net: split socket creation out of connection ced81907f7

net: move connection parameters into a struct and pass them all at once 38a58fd617

net: merge duplicate checks 4aeb685062

net: create callbacks for new connections and outgoing failures 03e73a734c

net: combine OpenNetworkConnection and ConnectNode 813de90e87

net: add RAII helper for outgoing connection failure callback d44e02bfef

net: use OnNewConnection for incoming connections as well c8efe8caff

net: when de-activating p2p, set nodes to be disconnected rather than closing the socket immediately

Messing with the socket from a non-network thread adds unnecessary complexity.

375a916275

net: Add a Disconnect function for nodes

For now, this just sets fDisconnect like before. But a function will be
necessary later.

72259b1e0c

scripted-diff: Call Disconnect rather than setting fDisconnect directly

-BEGIN VERIFY SCRIPT-
sed -i 's/fDisconnect = true/Disconnect()/' src/net.cpp src/net_processing.cpp
-END VERIFY SCRIPT-

df02ec6141

net: add an EnableReceive function so that the message handler can trigger it

for now this just disables fPauseRecv, but it will need to be a function later.

7bf4b56e58

net: get rid of optimistic send and the socket lock

The socket lock only existed because it was possible to trigger an immediate
disconnect via threads other than the socket handler.

The optimistic send was the last offender.

9b5b0a0a1e

net: move socket receiving to its own function 6eaae10aa2

net: narrow the scope for cs_vRecv to shared variables only a9f4dbdf3f

net: fix possibly skipped decrefs at shutdown 87aaa48274

net: move inactivity checking to its own function 17405ed1ad

net: pass the socket/whitelist args directly to AcceptConnection

Subsequent changes will turn AcceptConnection into a callback where the socket
is one of the supplied params.

f5ffe414f0

net: add an event handler for future read/write callbacks

For now this just handles read/write/error/timeout conditions for each node
serially, but OnEvents will soon be invoked as a callback when socket status
changes.

53d6bdfe82

net: hook up minimal libevent usage for connection acceptance de8b234222

net: Only close the socket in once place

By only allowing the socket to be closed and the handle to be invalidated in
one place, it becomes much easier to reason about the status at any given time.

c7f9e608e8

net: switch to libevent for node socket handling 863d4998ba

Net: handle all new connections on the socket handler thread

This ensures that outgoing connections don't race with event teardown at
shutdown.

f3fc360b8e

net: optimization: use scatter/gather to send up to 16 messages at once 77cee166e1

theuni force-pushed on Sep 20, 2017

theuni renamed this:
~~RFC: switch to libevent for node socket handling~~
WIP: switch to libevent for node socket handling
on Sep 20, 2017

promag commented at 9:19 PM on October 18, 2017: contributor

Needs rebase.

laanwj referenced this in commit ba2f19504c on Dec 13, 2017

maflcko added the label Needs rebase on Jun 6, 2018

DrahtBot commented at 11:20 PM on November 8, 2018: contributor

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

DrahtBot added the label Up for grabs on Nov 8, 2018

DrahtBot closed this on Nov 8, 2018

laanwj removed the label Needs rebase on Oct 24, 2019

PastaPastaPasta referenced this in commit 7f88a67d5b on Jan 25, 2020

gades referenced this in commit 652769becd on Jun 25, 2021

bitcoin locked this on Dec 16, 2021

fanquake removed the label Up for grabs on Jul 23, 2025

fanquake commented at 10:52 AM on July 23, 2025: member

Removing "Up for grabs", given we are now in the process of trying to remove libevent.