WIP: switch to libevent for node socket handling #11227

theuni · 2017-09-03T21:13:29Z

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 · 2017-09-04T00:16:34Z

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.

sipa · 2017-09-04T04:07:32Z

src/net.cpp

@@ -454,7 +454,7 @@ void CConnman::DumpBanlist()

 void CNode::CloseSocketDisconnect()
 {
-    fDisconnect = true;
+    Disconnect();


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

Probably so, will give it a shot.

sipa · 2017-09-04T06:20:24Z

src/net.cpp

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


This test is no longer needed?

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.

sipa · 2017-09-04T06:37:05Z

src/net.cpp

-        CloseSocket(hSocket);
-        return;
+    const char* method = event_base_get_method(m_event_base);
+    if (!method || (strncmp(method, "select", 6) == 0)) {


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

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.

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

sipa · 2017-09-04T07:16:31Z

src/net.cpp

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


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

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.

sipa · 2017-09-04T07:22:11Z

src/net.cpp

@@ -456,7 +471,14 @@ void CConnman::DumpBanlist()

 void CNode::CloseSocketDisconnect()
 {
-    Disconnect();
+    if (m_write_event) {


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

Agreed, see PR description :)

laanwj · 2017-09-05T21:56:02Z

Concept ACK, woohoo!

jimpo · 2017-09-05T23:00:14Z

src/net.cpp


+        }
+        WakeMessageHandler();


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

Yes, thank you.

sipa · 2017-09-05T23:18:10Z

src/net.cpp

@@ -1150,7 +1150,7 @@ void CConnman::AcceptConnection(const ListenSocket& hListenSocket) {
        }
    }

-    bool whitelisted = hListenSocket.whitelisted || IsWhitelistedRange(addr);
+    whitelisted = whitelisted || IsWhitelistedRange(addr);


||=

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

Oops, you're right. Should have checked.

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

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.

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

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

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

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

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.

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

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.

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.

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

promag · 2017-10-18T21:19:35Z

Needs rebase.

3830b6e net: use CreateSocket for binds (Cory Fields) df3bcf8 net: pass socket closing responsibility up to caller for outgoing connections (Cory Fields) 9e3b2f5 net: Move IsSelectableSocket check into socket creation (Cory Fields) 1729c29 net: split socket creation out of connection (Cory Fields) Pull request description: Requirement for #11227. We'll need to create sockets and perform the actual connect in separate steps, so break them up. #11227 adds an RAII wrapper around connection attempts, as a belt-and-suspenders in case a CloseSocket is missed. Tree-SHA512: de675bb718cc56d68893c303b8057ca062c7431eaa17ae7c4829caed119fa3f15b404d8f52aca22a6bca6e73a26fb79e898b335d090ab015bf6456cf417fc694

DrahtBot · 2018-11-08T23:20:41Z

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.

3830b6e net: use CreateSocket for binds (Cory Fields) df3bcf8 net: pass socket closing responsibility up to caller for outgoing connections (Cory Fields) 9e3b2f5 net: Move IsSelectableSocket check into socket creation (Cory Fields) 1729c29 net: split socket creation out of connection (Cory Fields) Pull request description: Requirement for bitcoin#11227. We'll need to create sockets and perform the actual connect in separate steps, so break them up. bitcoin#11227 adds an RAII wrapper around connection attempts, as a belt-and-suspenders in case a CloseSocket is missed. Tree-SHA512: de675bb718cc56d68893c303b8057ca062c7431eaa17ae7c4829caed119fa3f15b404d8f52aca22a6bca6e73a26fb79e898b335d090ab015bf6456cf417fc694

fanquake · 2025-07-23T10:52:32Z

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

theuni force-pushed the minimal-libevent3 branch from bc185e7 to 62d4d49 Compare September 3, 2017 22:30

theuni force-pushed the minimal-libevent3 branch from 62d4d49 to 7274a18 Compare September 4, 2017 02:02

fanquake added P2P Refactoring labels Sep 4, 2017

sipa reviewed Sep 4, 2017

View reviewed changes

jimpo reviewed Sep 5, 2017

View reviewed changes

sipa reviewed Sep 5, 2017

View reviewed changes

theuni force-pushed the minimal-libevent3 branch from 7274a18 to 8df8587 Compare September 12, 2017 18:41

theuni mentioned this pull request Sep 13, 2017

net: split resolve out of connect #10663

Merged

theuni force-pushed the minimal-libevent3 branch from 8df8587 to da4312d Compare September 19, 2017 02:23

theuni mentioned this pull request Sep 19, 2017

net: Split socket create/connect #11363

Merged

theuni added 17 commits September 19, 2017 21:50

net: split socket creation out of connection

ced8190

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

38a58fd

net: merge duplicate checks

4aeb685

net: create callbacks for new connections and outgoing failures

03e73a7

net: combine OpenNetworkConnection and ConnectNode

813de90

net: add RAII helper for outgoing connection failure callback

d44e02b

net: use OnNewConnection for incoming connections as well

c8efe8c

net: when de-activating p2p, set nodes to be disconnected rather than…

375a916

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

net: Add a Disconnect function for nodes

72259b1

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

scripted-diff: Call Disconnect rather than setting fDisconnect directly

df02ec6

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

net: add an EnableReceive function so that the message handler can tr…

7bf4b56

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

net: get rid of optimistic send and the socket lock

9b5b0a0

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.

net: move socket receiving to its own function

6eaae10

net: narrow the scope for cs_vRecv to shared variables only

a9f4dbd

net: fix possibly skipped decrefs at shutdown

87aaa48

net: move inactivity checking to its own function

17405ed

net: pass the socket/whitelist args directly to AcceptConnection

f5ffe41

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

theuni added 6 commits September 20, 2017 13:19

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

53d6bdf

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.

net: hook up minimal libevent usage for connection acceptance

de8b234

net: Only close the socket in once place

c7f9e60

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.

net: switch to libevent for node socket handling

863d499

Net: handle all new connections on the socket handler thread

f3fc360

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

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

77cee16

theuni force-pushed the minimal-libevent3 branch from da4312d to 77cee16 Compare September 20, 2017 20:41

theuni changed the title ~~RFC: switch to libevent for node socket handling~~ WIP: switch to libevent for node socket handling Sep 20, 2017

theuni mentioned this pull request Oct 18, 2017

Iterate and remove nodes without container copy #11514

Closed

theuni mentioned this pull request Feb 23, 2018

Net: Pass MSG_MORE flag when sending non-final network messages #12519

Closed

maflcko added the Needs rebase label Jun 6, 2018

DrahtBot added the Up for grabs label Nov 8, 2018

DrahtBot closed this Nov 8, 2018

laanwj removed the Needs rebase label Oct 24, 2019

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

fanquake removed the Up for grabs label Jul 23, 2025

WIP: switch to libevent for node socket handling #11227

WIP: switch to libevent for node socket handling #11227

Uh oh!

Conversation

theuni commented Sep 3, 2017 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

theuni commented Sep 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Sep 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag commented Oct 18, 2017

Uh oh!

DrahtBot commented Nov 8, 2018

Uh oh!

fanquake commented Jul 23, 2025

Uh oh!

Uh oh!

theuni commented Sep 3, 2017 •

edited by maflcko

Loading