-
Notifications
You must be signed in to change notification settings - Fork 1.2k
backport: merge bitcoin#24356 (replace CConnman::SocketEvents() with mockable Sock::WaitMany()), implement Sock::WaitMany{Epoll,KQueue}()
#6018
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
-socketevents to Sock, implement Sock::WaitMany{Epoll, KQueue} -socketevents to Sock, implement Sock::WaitMany{Epoll, KQueue}
, bitcoin#22829, bitcoin#24079, bitcoin#24108, bitcoin#24157, bitcoin#25109 (network backports: part 5) 5dde8e7 merge bitcoin#25109: Strengthen AssertLockNotHeld assertions (Kittywhiskers Van Gogh) a1f005e merge bitcoin#24157: Replace RecursiveMutex cs_totalBytesSent with Mutex and rename it (Kittywhiskers Van Gogh) de4b4bf merge bitcoin#24108: Replace RecursiveMutex cs_addrLocal with Mutex, and rename it (Kittywhiskers Van Gogh) 2f7a138 merge bitcoin#24079: replace RecursiveMutex cs_SubVer with Mutex (and rename) (Kittywhiskers Van Gogh) 23b152c merge bitcoin#22829: various RecursiveMutex replacements in CConnman (Kittywhiskers Van Gogh) 362e310 merge bitcoin#21943: Dedup and RAII-fy the creation of a copy of CConnman::vNodes (Kittywhiskers Van Gogh) bf98ad6 merge bitcoin#22782: Remove unused MaybeSetAddrName (Kittywhiskers Van Gogh) 2b65526 merge bitcoin#21167: make CNode::m_inbound_onion public, initialize explicitly (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependent on #6001 * Dependency for #6018 * Partially reverts ff69e0d from #5336 due to `Span<CNode*>`'s incompatibility with `CConnman::NodesSnapshot::Snap()` (returning `const std::vector<CNode*>&`) ``` masternode/sync.cpp:147:18: error: no matching member function for call to 'RequestGovernanceObjectVotes' m_govman.RequestGovernanceObjectVotes(snap.Nodes(), connman); ~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~ ./governance/governance.h:360:9: note: candidate function not viable: no known conversion from 'const std::vector<CNode *>' to 'CNode &' for 1st argument int RequestGovernanceObjectVotes(CNode& peer, CConnman& connman) const; ^ ./governance/governance.h:361:9: note: candidate function not viable: no known conversion from 'const std::vector<CNode *>' to 'Span<CNode *>' for 1st argument int RequestGovernanceObjectVotes(Span<CNode*> vNodesCopy, CConnman& connman) const; ^ 1 error generated. ``` * Dash already implements its own `CNode*` iteration logic in [dash#1382](#1382) and implemented additional capabilities in [dash#1575](#1575), which meant backporting [bitcoin#21943](bitcoin#21943) involved migrating Dash-specific code to upstream logic that needed to be modified to implement expected functionality. * Unlike Bitcoin, Dash maintains a map of every raw `SOCKET` corresponding to a pointer of their `CNode` instance and uses it to translate socket sets to their corresponding `CNode*` sets. This is done to accommodate for edge-triggered modes which have an event-socket relationship, as opposed to level-triggered modes, which have a socket-event relationship. This means that `CConnman::SocketHandlerConnected()` doesn't require access to a vector of all `CNode` pointers and therefore, the argument `nodes` has been omitted. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK 5dde8e7 Tree-SHA512: 5685d8ebb4fa1f10d018e60d9b0efc3100ea13ac437e7892a09ad3f86d6ac6756e4b5a08ebe70de2eabb27740678e10b975d319f2d553ae5b27dafa71dba0a9f
…keup pipes logic out of `CConnman` and into `EdgeTriggeredEvents` and `WakeupPipes` bd8b5d4 net: add more details to log information in ETE and `WakeupPipes` (Kittywhiskers Van Gogh) ec99294 net: restrict access `EdgeTriggerEvents` members (Kittywhiskers Van Gogh) f24520a net: log `close` failures in `EdgeTriggerEvents` and `WakeupPipe` (Kittywhiskers Van Gogh) b8c3b48 refactor: introduce `WakeupPipe`, move wakeup select pipe logic there (Kittywhiskers Van Gogh) ed7d976 refactor: move wakeup pipe (de)registration to ETE (Kittywhiskers Van Gogh) f50c710 refactor: move `CConnman::`(`Un`)`registerEvents` to ETE (Kittywhiskers Van Gogh) 3a9f386 refactor: move `SOCKET` addition/removal from interest list to ETE (Kittywhiskers Van Gogh) 212df06 refactor: introduce `EdgeTriggeredEvents`, move {epoll, kqueue} fd there (Kittywhiskers Van Gogh) 3b11ef9 refactor: move `CConnman::SocketEventsMode` to `util/sock.h` (Kittywhiskers Van Gogh) Pull request description: ## Motivation `CConnman` is an entity that contains a lot of platform-specific implementation logic, both inherited from upstream and added upon by Dash (support for edge-triggered socket events modes like `epoll` on Linux and `kqueue` on FreeBSD/Darwin). Bitcoin has since moved to strip down `CConnman` by moving peer-related logic to the `Peer` struct in `net_processing` (portions of which are backported in #5982 and friends, tracking efforts from bitcoin#19398) and moving socket-related logic to `Sock` (portions of which are aimed to be backported in #6004, tracking efforts from bitcoin#21878). Due to the direction being taken and the difference in how edge-triggered events modes operate (utilizing interest lists and events instead of iterating over each socket) in comparison to level-triggered modes (which are inherited from upstream), it would be reasonable to therefore, isolate Dash-specific code into its own entities and minimize the information `CConnman` has about its internal workings. One of the visible benefits of this approach is comparing `develop` (as of this writing, d44b0d5) and this pull request for interactions between wakeup pipes logic and {`epoll`, `kqueue`} logic. This is what construction looks like: https://github.com/dashpay/dash/blob/d44b0d5dcb9b54821d582b267a8b92264be2da1b/src/net.cpp#L3358-L3397 But, if we segment wakeup pipes logic (that work on any platform with POSIX APIs and excludes Windows) and {`epoll`, `kqueue`} logic (calling them `EdgeTriggeredEvents` instead), construction looks different: https://github.com/dashpay/dash/blob/907a3515170abed4ce9018115ed591e6ca9f4800/src/util/wpipe.cpp#L12-L38 Now wakeup pipes logic doesn't need to know what socket events mode is being used nor are the implementation aspects of (de)registering it its concern, that is now `EdgeTriggeredEvents` problem. ## Additional Information * This pull request will need testing on macOS (FreeBSD isn't a tier-one target) to ensure that lack of breakage in `kqueue`-specific logic. ## Breaking Changes * Dependency for #6018 * More logging has been introduced and existing log messages have been made more exhaustive. If there is parsing that relies on a particular template, they will have to be updated. * If `EdgeTriggeredEvents` or `WakeupPipes` fail to initialize or are incorrectly initialized and not destroyed immediately, any further attempts at calling any of its functions will result in an `assert`-induced crash. Earlier behavior may have allowed for silent failure but segmentation of logic from `CConnman` means the newly created instances must only exist if the circumstances needed for it to initialize correctly are present. This is to ensure that `CConnman` doesn't have to concern itself with internal workings of either entities. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)** - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK bd8b5d4 Tree-SHA512: 8f793d4b4f2d8091e05bb9cc108013e924bbfbf19081290d9c0dfd91b0f2c80652ccf853f1596562942b4433509149c526e111396937988db605707ae1fe7366
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few suggestions. also, kqueue still doesn't work :/
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few more suggestions
|
This pull request has conflicts, please rebase. |
, bitcoin#25426, bitcoin#24378 (sockets backports) c24804c merge bitcoin#24378: make bind() and listen() mockable/testable (Kittywhiskers Van Gogh) be19868 merge bitcoin#25426: add new method Sock::GetSockName() that wraps getsockname() and use it in GetBindAddress() (Kittywhiskers Van Gogh) 6b159f1 merge bitcoin#24357: make setsockopt() and SetSocketNoDelay() mockable/testable (Kittywhiskers Van Gogh) 9c751ef merge bitcoin#23604: Use Sock in CNode (Kittywhiskers Van Gogh) 508044c merge bitcoin#21879: wrap accept() and extend usage of Sock (Kittywhiskers Van Gogh) Pull request description: ## Additional Information * Dependency for #6018 ## Breaking Changes None expected. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)** - [x] I have added or updated relevant unit/integration/functional/e2e tests - [x] I have made corresponding changes to the documentation **(note: N/A)** - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: PastaPastaPasta: utACK c24804c Tree-SHA512: 5149de0f1983bb56517c30b31d137b33b8a49b0e695be2dada71ff3e3bb22908556db343391b7df7e3c7c2ed60ae1fc11a4f4af4f47e35a2a1d3ce7463c03d41
-socketevents to Sock, implement Sock::WaitMany{Epoll, KQueue} -socketevents to Sock, implement Sock::WaitMany{Epoll, KQueue}
|
This pull request has conflicts, please rebase. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kqueue seems to be working now 👍
light ACK 67151dc
|
This pull request has conflicts, please rebase. |
No behavior has changed as we don't support any additional APIs and SEM_LT_DEFAULT preserves old behavior but upcoming commits will utilize this to able to effectuate the preferences set by `-socketevents`.
`IsSelectableSocket()` was subsumed into `Sock` for mockability's sake but creates problems when our event-socket map uses the raw `SOCKET` and not `Sock`, so we need to bring it back. On top of that, `IsSelectableSocket()`'s behavior was defined at compile-time when it should've been sensitive to the runtime capabilities we give it. Currently, this does not cause a change in behavior but makes way for future changes.
…ock::WaitMany() Co-authored-by: UdjinM6 <[email protected]> continuation of 8b23bfb from dash#6630
Co-authored-by: UdjinM6 <[email protected]>
We handle timeouts in `CConnman::SocketHandler()`, so there is no adverse effects for bailing out early from `poll` or `select` when there's nothing to monitor to begin with. Co-authored-by: UdjinM6 <[email protected]>
Graceful fallback was necessary when filling in the gaps functionality wise but now that everything is in place, we should revert back to the old behavior of crashing when supplied an invalid or inapplicable SocketEventsMode preference as this means we didn't filter out an invalid input from the user-end or didn't set our defaults correctly, which is a logic error.
This was acceptable while we were refining earlier commits but default args could in combination with future backports result in behavior changes that were unanticipated, we should clear the air right now. We are introducing a new global, `g_socket_events_mode` to allow us to influence Wait(Many)() calls as and when needed. A global is acceptable as it's lightweight, a parameter that is not configurable during runtime except during initial startup and because the structure in which it is ordinarily stored is far heavier. Co-authored-by: UdjinM6 <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebase looks clean
re-utACK c611fb0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK c611fb0
Additional information
Dependent on backport: merge bitcoin#21167, #22782, #21943, #22829, #24079, #24108, #24157, #25109 (network backports: part 5) #6004
Dependent on refactor: move {
epoll,kqueue} (de)init logic and wakeup pipes logic out ofCConnmanand intoEdgeTriggeredEventsandWakeupPipes#6007Dependent on fix: actually use
-socketevents#6027Deviations from upstream
EventsPerSockis a unordered map ofshared_ptrs ofSockwrappers andEventsEventsPerSockis an unordered map of raw socket file descriptors (SOCKET) andEventsWakeupPipes, which is constructed and destroyed using an entity outsideSock's control. We need to be able to insert the read pipe raw socket into equivalent of therecvsocket set and query for it later on.It would be technically possible, though cumbersome, to wrap the read pipe raw socket in a
Sockand overwrite the destructor if it wasn't for the support of edge-triggered modes which have an event-socket relationship, as opposed to level triggered modes, that have a socket-event relationship.EventsPerSockmap will always return with event data for every corresponding entry.EventsPerSockmap may return with event data for its corresponding entry.pollorselect. Otherwise, it will behave as described.This is due to the inversion of the socket-event relationship in edge-triggered modes (
epollandkqueue), as alluded to earlier. As edge-triggered modes return events and their corresponding socket (sockets registered throughEdgeTriggeredEvents::RegisterEntity()and friends), theEventsPerSockmap, will have its contents completely discarded and substituted with the results of {epoll,kqueue}.Sockentity to callSock::WaitMany()Sock::WaitMany()'s underlying logic through callingSock::WaitManyInternal()(and access any specific event mode's implementation) without aSockentity.WaitManyby seeking to the first element to access it. This was possible because the unordered map consisted ofSockentities. As that isn't the case for Dash andWaitManydoesn't truly rely on instance-specific member values of a particularSockinstance (the values it relies on should remain constant throughout program runtime), it can be safely made astaticfunction and that was exactly what was done.It has been named
WaitManyInternal()as one ofSock's purposes is mockability andWaitMany()(simply a passthrough toWaitManyInternal()) has been defined as avirtualfunction.Sock's usage of platform-specific APIs is decided exclusively at compile-time.Sock's usage of platform-specific APIs is determined by what is supported at compile-time and decided at runtime (mostly).Sock::Wait()(which is transformed intoSock::WaitMany()in this pull request) supported onlypollandselectand behaved as described for Bitcoin.The described behaviour for Dash was only applicable for
CConnman::SocketEvents(). But, asSocketEvents()is being replaced wholesale withWaitMany(),WaitMany()needed to be adapted to mirrorSocketEvents()behaviour.This has resulted in changes that now require knowledge of the expected runtime SEM and file descriptor (if using an edge-triggered mode).
Sock::Wait()andSock::WaitMany()behave identicallySock::Wait()will respect the SEM selection argument if it is level-triggered but will fallback topollorselect(determined at compile-time) if the SEM selection is edge-triggered.Because of that and a) the unlikelihood of the socket probed being registered with
EdgeTriggeredEvents::RegisterEntity()and b) the overhead involved in fetching a list, filtering out for the particular socket we care about and flagging the result, it is more practical to use an LT-SEM instead.How Has This Been Tested?
Correctness of
socket,pollandepollSEMs were tested using a Debian 12 (bookworm) Docker container with additional logging to ensure the correct syscalls were being made. Correctness of thekqueueSEM was tested using a GhostBSD 23.10.1 (based on FreeBSD 13.2-STABLE) virtual machine with similar additional logging.Breaking Changes
None expected. Behaviour should remain unchanged.
Checklist
Go over all the following points, and put an
xin all the boxes that apply.