Skip to content

Commit 3b0323a

Browse files
Merge dashpay#6007: refactor: move {epoll, kqueue} (de)init logic and wakeup 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 dashpay#5982 and friends, tracking efforts from bitcoin#19398) and moving socket-related logic to `Sock` (portions of which are aimed to be backported in dashpay#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 dashpay#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
2 parents 146be9f + bd8b5d4 commit 3b0323a

File tree

10 files changed

+600
-312
lines changed

10 files changed

+600
-312
lines changed

src/Makefile.am

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,7 @@ BITCOIN_CORE_H = \
327327
util/bip32.h \
328328
util/bytevectorhash.h \
329329
util/check.h \
330+
util/edge.h \
330331
util/enumerate.h \
331332
util/epochguard.h \
332333
util/error.h \
@@ -362,6 +363,7 @@ BITCOIN_CORE_H = \
362363
util/ui_change_type.h \
363364
util/url.h \
364365
util/vector.h \
366+
util/wpipe.h \
365367
validation.h \
366368
validationinterface.h \
367369
versionbits.h \
@@ -776,6 +778,7 @@ libbitcoin_util_a_SOURCES = \
776778
util/bip32.cpp \
777779
util/bytevectorhash.cpp \
778780
util/check.cpp \
781+
util/edge.cpp \
779782
util/error.cpp \
780783
util/fees.cpp \
781784
util/hasher.cpp \
@@ -795,6 +798,7 @@ libbitcoin_util_a_SOURCES = \
795798
util/thread.cpp \
796799
util/threadnames.cpp \
797800
util/tokenpipe.cpp \
801+
util/wpipe.cpp \
798802
$(BITCOIN_CORE_H)
799803

800804
if USE_LIBEVENT

src/init.cpp

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2515,23 +2515,9 @@ bool AppInitMain(const CoreContext& context, NodeContext& node, interfaces::Bloc
25152515
}
25162516
}
25172517

2518-
std::string strSocketEventsMode = args.GetArg("-socketevents", DEFAULT_SOCKETEVENTS);
2519-
if (strSocketEventsMode == "select") {
2520-
connOptions.socketEventsMode = CConnman::SOCKETEVENTS_SELECT;
2521-
#ifdef USE_POLL
2522-
} else if (strSocketEventsMode == "poll") {
2523-
connOptions.socketEventsMode = CConnman::SOCKETEVENTS_POLL;
2524-
#endif
2525-
#ifdef USE_EPOLL
2526-
} else if (strSocketEventsMode == "epoll") {
2527-
connOptions.socketEventsMode = CConnman::SOCKETEVENTS_EPOLL;
2528-
#endif
2529-
#ifdef USE_KQUEUE
2530-
} else if (strSocketEventsMode == "kqueue") {
2531-
connOptions.socketEventsMode = CConnman::SOCKETEVENTS_KQUEUE;
2532-
#endif
2533-
} else {
2534-
return InitError(strprintf(_("Invalid -socketevents ('%s') specified. Only these modes are supported: %s"), strSocketEventsMode, GetSupportedSocketEventsStr()));
2518+
std::string sem_str = args.GetArg("-socketevents", DEFAULT_SOCKETEVENTS);
2519+
if (SEMFromString(sem_str) == SocketEventsMode::Unknown) {
2520+
return InitError(strprintf(_("Invalid -socketevents ('%s') specified. Only these modes are supported: %s"), sem_str, GetSupportedSocketEventsStr()));
25352521
}
25362522

25372523
const std::string& i2psam_arg = args.GetArg("-i2psam", "");

0 commit comments

Comments
 (0)