Give CValidationInterface Support for calling notifications on the CScheduler Thread #10179

TheBlueMatt · 2017-04-10T20:43:19Z

Apologies for the plumbing-only no-changes PR, but the "move wallet updates out of cs_main into a background thread" changeset is a bit big for all one go, so instead I'm doing this first. It simply gives CValidationInterface the neccessary plumbing to handle callbacks on the CScheduler thread. See TheBlueMatt@4e82e40 for a commit which switches a callback into the background thread.

The CScheduler thread was super lonely, so I decided to use that instead of adding new threads.

This conflicts trivially with #10178, but not enough to avoid doing parallel reviews. After that and this are merged, "move wallet callbacks into background thread" is just one more (somewhat large, sadly) PR away. See https://github.com/TheBlueMatt/bitcoin/commits/2017-01-wallet-cache-inmempool for the full branch.

TheBlueMatt · 2017-04-17T13:54:00Z

Rebased after ##10178 merge.

morcos · 2017-04-24T18:10:57Z

src/validationinterface.cpp

+    // our own queue here :(
+    CCriticalSection cs_callbacksPending;
+    std::list<std::function<void (void)>> callbacksPending;
+    std::atomic<bool> fCallbacksRunning = ATOMIC_VAR_INIT(false);


nit: i'm the last person who should comment on coding style, but isn't this easier to read:
std::atomic<bool> fCallbacksRunning(false)

My C++-fu is not good enough to get that to compile. Not actually sure why, though, frankly.

Could you try

std::atomic<bool> fCallbacksRunning = std::atomic<bool>(false);

?

Yes, that works, but I think thats worse than using the init wrapper, which is what that wrapper exists for.

Use braced initialization in a class/struct definition:

std::atomic<bool> fCallbacksRunning{false};

Rewrote to just use a regular bool inside the lock, no reason to really have an atomic here.

morcos · 2017-04-24T18:11:21Z

fast review utACK

laanwj · 2017-04-25T12:55:29Z

utACK 010f3ae

TheBlueMatt · 2017-04-25T12:58:28Z

I don't currently have tests written, but given nearly everything passes through I've found its pretty well-excersized by existing wallet functional tests. I'll add unit tests for this to my to-do list.

…

On April 25, 2017 8:55:34 AM EDT, "Wladimir J. van der Laan" ***@***.***> wrote: utACK e5d0c66 - are you planning on adding tests for this functionality later?

laanwj · 2017-04-25T13:06:35Z

src/validationinterface.cpp

+                callback = callbacksPending.front();
+                callbacksPending.pop_front();
+            }
+            callback();


This doesn't seem exception-safe. If an exception is raised inside here, fCallbacksRunning will stay set forever.

Indeed. Put a generic try {} catch(...) {Log} around it, not sure what else to do there.

You could catch the exception, clear the flag, and throw again. Though RAII is usually the best way in C++ to cover all exit paths.

OK, made a local class that will RAII it :)

sipa

I'll give you the same comment as you gave on #10148... you're adding unused and untested functionality.

The first commit seems a useful refactoring that could be its own PR, but the rest seem to be preparations for something that doesn't exist yet.

sipa · 2017-04-26T00:59:08Z

src/validationinterface.cpp

+}
+
+CMainSignals::~CMainSignals() {
+    delete internals;


Use a std::unique_ptr instead?

Cant, sadly, as the whole point was to not make CMainSignalsInterface sit in the .h to avoid having a bost/signals include in half our codebase.

std::unique_ptr seems to work fine with forward-declared types: sipa@a4ecaad

Well, I'll be! Took your patch and squashed (hope you dont mind).

Sorry, but I'm going to insist that you maintain this commit intact (nevermind that it's likely the exact same patch you'd come up with if I hadn't shown you) forever in your PR history. Note: sarcasm.

sipa · 2017-04-26T01:00:50Z

src/validationinterface.cpp

+    }
+
+    void AddToProcessQueue(const std::function<void (void)>& func) {
+        if (!scheduler)


That's scary, it would lose a callback. Shouldn't it assert in this case?

sipa · 2017-04-26T01:03:04Z

src/validationinterface.cpp

+            return;
+        }
+
+        while (true) {


Instead of looping until there are no more scheduled callbacks, isn't easier/better to return, but reschedule itself? That way a large set of queued callbacks won't prevent the scheduler from running other (non-callback) jobs in the mean time.

TheBlueMatt · 2017-04-27T01:00:00Z

@sipa re: unused and untested code, see #10286, which I think it would be reasonable to require at least concept acks on prior to merging this.

ryanofsky

utACK 4fceec59372b778f9966485f2a13f97e21b1db4c

ryanofsky · 2017-05-02T16:43:09Z

src/validationinterface.h

+struct CMainSignalsInstance;
+class CMainSignals {
+private:
+    std::unique_ptr<CMainSignalsInstance> internals;


In commit "Make ValidationInterface signals-type-agnostic"

I don't think you need to have this internals pointer in order to do the signal routing / CScheduler stuff that's ostensibly the motivation for this change. (The signals could just be regular class members.). If you like this better because it helps hide the boost dependency, though, that seems fine. This is just the pImpl pattern, http://stackoverflow.com/questions/8972588/is-the-pimpl-idiom-really-used-in-practice.

Yea, the real motivation there was to not have to #include boost signals garbage everywhere, nothing more.

ryanofsky · 2017-05-02T17:09:49Z

src/validationinterface.cpp

+    // We are not allowed to assume the scheduler only runs in one thread,
+    // but must ensure all callbacks happen in-order, so we end up creating
+    // our own queue here :(
+    CCriticalSection cs_callbacksPending;


In commit "Handle more than one CScheduler thread in CValidationInterface"

This seems like it would a lot simpler it just spawned a single thread to run the callbacks in sequence instead of relying on CScheduler and then doing a bunch of synchronization on top of it to keep things scheduled and prevent more than one callback from executing at the same time.

Also, I think it would be nice if this functionality were implemented in a standalone class that could be unit tested.

Anyway, implementation seems fine, I couldn't find any bugs.

Moved to scheduler.h, testing left as an excersize for the reader :p.

ryanofsky · 2017-05-02T17:19:07Z

src/validationinterface.cpp

+        callback();
+    }
+
+    void AddToProcessQueue(const std::function<void (void)>& func) {


In commit "Handle more than one CScheduler thread in CValidationInterface":

Should pass func by value or rvalue reference so you can move it into the callbacksPending list without a copy.

ryanofsky · 2017-05-02T17:21:03Z

src/validationinterface.cpp

+            if (callbacksPending.empty()) return;
+            fCallbacksRunning = true;
+
+            callback = callbacksPending.front();


In commit "Handle more than one CScheduler thread in CValidationInterface"

Could add std::move here.

ryanofsky · 2017-05-03T13:43:59Z

utACK 643a988101f12581725d08d71cfcb5d932a062ac. I like the new ProcessQueue location and std::move additions.

TheBlueMatt · 2017-05-04T20:51:49Z

Squashed and tweaked commit wording.

ryanofsky · 2017-05-05T20:51:53Z

utACK 830c26b55a2b4bc93509d3fa7f17a0b383a95835. Only change aside from the history squashing is switching to the schedule() default arg.

sipa · 2017-06-03T19:39:21Z

utACK. Since you're adding a new class, would you mind making a few changes to match the new naming rules in the style guide?

Note that the CScheduler thread cant be running at this point, it has already been stopped with the rest of the init threadgroup. Thus, just calling any remaining loose callbacks during Shutdown() is sane.

TheBlueMatt · 2017-07-07T16:56:47Z

Added a comment on flush/drop behavior -
"Any future callbacks will be dropped. This should absolutely be safe - if missing a callback results in an unrecoverable situation, unclean shutdown would too. The only reason to do the above flushes is to let the wallet catch up with our current chain to avoid any strange pruning edge cases and make next startup faster by avoiding rescan."

morcos · 2017-07-07T17:17:37Z

utACK 3192975

theuni · 2017-07-07T23:38:31Z

re-utACK 3192975, though very hesitantly after thinking through the tear-down some more. The shutdown process is going to get really hard to trace when callbacks start coming in on different threads.

I really hope some ownership model will begin to emerge.

ryanofsky

utACK 3192975. Changes since last review were rebase after multiwallet & ScriptForMining removal, and adding the flush commit.

I left a few suggestions pertaining to the flush commit. I think this code would be good to clean up at some point, though it don't think it should hold up the PR.

ryanofsky · 2017-07-10T15:35:20Z

src/validationinterface.cpp

@@ -44,6 +44,10 @@ void CMainSignals::UnregisterBackgroundSignalScheduler() {
    m_internals.reset(nullptr);
 }

+void CMainSignals::FlushBackgroundCallbacks() {
+    m_internals->m_schedulerClient.EmptyQueue();


In commit "Flush CValidationInterface callbacks prior to destruction"

Shutdown order is convoluted enough that I think it would be good to add asserts, like asserting m_internals is non-null here and that scheduler is shut down or shutting down at this point (nThreadsServicingQueue == 0 || shouldStop). Latter might be more appropriate inside the scheduling code.

ryanofsky · 2017-07-10T16:32:59Z

src/init.cpp

@@ -251,6 +251,7 @@ void Shutdown()
    }
 #endif
    UnregisterAllValidationInterfaces();
+    GetMainSignals().UnregisterBackgroundSignalScheduler();


In commit "Give CMainSignals a reference to the global scheduler"

It seems like the right place for this unregister call would be earlier in Shutdown(), after scheduler thread is cancelled and the last signal is sent, for consistency with the register call, which is made when the scheduler thread is started.

This would let you flush the background queue when the signal scheduler is destroyed and not need separate FlushBackgroundCallbacks and EmptyQueue calls made in advance.

Also this would bring shutdown code closer to an raii-style ordering, which should make cleanup easier in the future and would make the various object lifetimes a little easier to understand now.

We cant unregister the background scheduler from the validation interface until we're sure nothing is gonna generate anymore callbacks (so, really, after the pcoinsTip/etc deletions). If we want to mirror the initialization order, we'd have to move it even further down, not up, as de-init in RAII order would be after wallet deltion.

ryanofsky · 2017-07-10T17:09:05Z

src/scheduler.h

@@ -101,6 +101,9 @@ class SingleThreadedSchedulerClient {
 public:
    SingleThreadedSchedulerClient(CScheduler *pschedulerIn) : m_pscheduler(pschedulerIn) {}
    void AddToProcessQueue(std::function<void (void)> func);
+
+    // Processes all remaining queue members on the calling thread, blocking until queue is empty


In commit "Flush CValidationInterface callbacks prior to destruction"

Comment should point out that this should only be called when scheduler is not active. Otherwise this could burn cpu racing with scheduler thread to run the next callback.

Added the assert instead

ryanofsky · 2017-07-10T17:44:29Z

src/validationinterface.cpp

 }

 void CMainSignals::UnregisterBackgroundSignalScheduler() {
-    m_internals->m_scheduler = NULL;
+    m_internals.reset(nullptr);


In commit "Support more than one CScheduler thread for serial clients"

It'd be good to just reset the scheduler pointer here instead of going overboard and destroying all the boost signals at this point as well. It just seems like a random and unexpected thing to be doing in a function called UnregisterBackgroundSignalScheduler especially given new flush behavior which allows signals be forwarded after the scheduler is stopped.

…ions on the CScheduler Thread 1f668b6 Expose if CScheduler is being serviced, assert its not in EmptyQueue (Matt Corallo) 3192975 Flush CValidationInterface callbacks prior to destruction (Matt Corallo) 08096bb Support more than one CScheduler thread for serial clients (Matt Corallo) 2fbf2db Add default arg to CScheduler to schedule() a callback now (Matt Corallo) cda1429 Give CMainSignals a reference to the global scheduler (Matt Corallo) 3a19fed Make ValidationInterface signals-type-agnostic (Matt Corallo) ff6a834 Use TestingSetup to DRY qt rpcnestedtests (Matt Corallo) Tree-SHA512: fab91e34e30b080ed4d0a6d8c1214910e383c45440676e37be61d0bde6ae98d61e8903d22b846e95ba4e73a6ce788798350266feba246d8a2ab357e8523e4ac5

…ut cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on #10179, this effectively reverts #9583, regaining most of the original speedups of #7946. This concludes the work of #9725, #10178, and #10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

…tifications on the CScheduler Thread 1f668b6 Expose if CScheduler is being serviced, assert its not in EmptyQueue (Matt Corallo) 3192975 Flush CValidationInterface callbacks prior to destruction (Matt Corallo) 08096bb Support more than one CScheduler thread for serial clients (Matt Corallo) 2fbf2db Add default arg to CScheduler to schedule() a callback now (Matt Corallo) cda1429 Give CMainSignals a reference to the global scheduler (Matt Corallo) 3a19fed Make ValidationInterface signals-type-agnostic (Matt Corallo) ff6a834 Use TestingSetup to DRY qt rpcnestedtests (Matt Corallo) Tree-SHA512: fab91e34e30b080ed4d0a6d8c1214910e383c45440676e37be61d0bde6ae98d61e8903d22b846e95ba4e73a6ce788798350266feba246d8a2ab357e8523e4ac5

… (without cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946. This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

…hread 6cecb7b AddToWalletIfInvolvingMe should test pIndex, not posInBlock (furszy) 73499ff Add missing lock in CScheduler::AreThreadsServicingQueue() (Matt Corallo) 6d1a60e ValidationInterface compiler warning fix (furszy) 9e523a7 Fix shutdown in case of errors during initialization (Wladimir J. van der Laan) 207c17f Expose if CScheduler is being serviced, assert its not in EmptyQueue (Matt Corallo) 64c525b Otherwise threads will try to continue modifying data structures while the dump process is being executed. (Matt Corallo) 8022463 Support more than one CScheduler thread for serial clients (Matt Corallo) 6532fca Add default arg to CScheduler to schedule() a callback now (Matt Corallo) d583d12 Give CMainSignals a reference to the global scheduler so that it can run some signals in the background later (Matt Corallo) c26d18c Remove unused NotifyTransactionLock signal. (furszy) 2c9de45 Remove unused UpdatedTransaction signal and connected slot. (furszy) 8f38657 make ConnectBlock static in validation.cpp (furszy) Pull request description: Another step forward in the wallet signals processing in the background thread + node validation <--> wallet separation long working paths. Adapting: * bitcoin#10178 * bitcoin#10179 * bitcoin#10221 * bitcoin#10914 * bitcoin#11783 * Plus added an extra cleanup to the validation interface removing unused `NotifyTransactionLock` signal and its respective slots. ACKs for top commit: Fuzzbawls: ACK 6cecb7b random-zebra: ACK 6cecb7b and merging... Tree-SHA512: ac1c128cb3e4ec47c2803da68ca4d4b16164687fc790f3cc1d0761e73bd61b4edc681e2ce045e52564dd13a8279495e9058a6aae316196bd7ad9c97a07dd0b20

… (without cs_main) 89f0312 Remove redundant pwallet nullptr check (Matt Corallo) c4784b5 Add a dev notes document describing the new wallet RPC blocking (Matt Corallo) 3ea8b75 Give ZMQ consistent order with UpdatedBlockTip on scheduler thread (Matt Corallo) cb06edf Fix wallet RPC race by waiting for callbacks in sendrawtransaction (Matt Corallo) e545ded Also call other wallet notify callbacks in scheduler thread (Matt Corallo) 17220d6 Use callbacks to cache whether wallet transactions are in mempool (Matt Corallo) 5d67a78 Add calls to CWallet::BlockUntilSyncedToCurrentChain() in RPCs (Matt Corallo) 5ee3172 Add CWallet::BlockUntilSyncedToCurrentChain() (Matt Corallo) 0b2f42d Add CallFunctionInQueue to wait on validation interface queue drain (Matt Corallo) 2b4b345 Add ability to assert a lock is not held in DEBUG_LOCKORDER (Matt Corallo) 0343676 Call TransactionRemovedFromMempool in the CScheduler thread (Matt Corallo) a7d3936 Add a CValidationInterface::TransactionRemovedFromMempool (Matt Corallo) Pull request description: Based on bitcoin#10179, this effectively reverts bitcoin#9583, regaining most of the original speedups of bitcoin#7946. This concludes the work of bitcoin#9725, bitcoin#10178, and bitcoin#10179. See individual commit messages for more information. Tree-SHA512: eead4809b0a75d1fb33b0765174ff52c972e45040635e38cf3686cef310859c1e6b3c00e7186cbd17374c6ae547bfbd6c1718fe36f26c76ba8a8b052d6ed7bc9

fanquake added the Validation label Apr 10, 2017

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-3 branch from 36ec5ee to 6d4e7db Compare April 17, 2017 13:53

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-3 branch from 6d4e7db to e5d0c66 Compare April 17, 2017 13:55

morcos reviewed Apr 24, 2017

View reviewed changes

laanwj reviewed Apr 25, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-3 branch from e5d0c66 to 1bcb07f Compare April 25, 2017 14:42

sipa reviewed Apr 26, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-3 branch 9 times, most recently from 4e96a11 to 4fceec5 Compare April 27, 2017 00:14

TheBlueMatt mentioned this pull request Apr 27, 2017

Call wallet notify callbacks in scheduler thread (without cs_main) #10286

Merged

ryanofsky reviewed May 2, 2017

View reviewed changes

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-3 branch from d3f0ad3 to 830c26b Compare May 4, 2017 20:51

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-3 branch from 453c2fd to c45d59c Compare July 7, 2017 15:52

Flush CValidationInterface callbacks prior to destruction

3192975

Note that the CScheduler thread cant be running at this point, it has already been stopped with the rest of the init threadgroup. Thus, just calling any remaining loose callbacks during Shutdown() is sane.

TheBlueMatt force-pushed the 2017-01-wallet-cache-inmempool-3 branch from c45d59c to 3192975 Compare July 7, 2017 16:56

ryanofsky reviewed Jul 10, 2017

View reviewed changes

Expose if CScheduler is being serviced, assert its not in EmptyQueue

1f668b6

laanwj merged commit 1f668b6 into bitcoin:master Jul 11, 2017

theuni mentioned this pull request Jul 11, 2017

net processing: swap out signals for an interface class #10756

Merged

TheBlueMatt mentioned this pull request Jul 16, 2017

Drop upgrade-cancel callback registration for a generic "cancelable" #10770

Merged

codablock mentioned this pull request Mar 15, 2019

Backports 0.15 pr8 dashpay/dash#2763

Merged

PastaPastaPasta mentioned this pull request Sep 7, 2019

Backport 0.15 pr25 dashpay/dash#3080

Merged

furszy mentioned this pull request Nov 15, 2020

[WIP][Validation] Back ports up to "bye bye" boost signal usages. PIVX-Project/PIVX#1973

Closed

furszy mentioned this pull request Feb 6, 2021

Support for validation interface signals in background thread PIVX-Project/PIVX#2192

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Give CValidationInterface Support for calling notifications on the CScheduler Thread #10179

Give CValidationInterface Support for calling notifications on the CScheduler Thread #10179

Uh oh!

Conversation

TheBlueMatt commented Apr 10, 2017

Uh oh!

TheBlueMatt commented Apr 17, 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!

morcos commented Apr 24, 2017

Uh oh!

laanwj commented Apr 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheBlueMatt commented Apr 25, 2017 via email

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!

sipa left a comment

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!

TheBlueMatt commented Apr 27, 2017

Uh oh!

ryanofsky left a comment

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!

ryanofsky May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ryanofsky commented May 3, 2017

laanwj commented Apr 25, 2017 •

edited

Loading

ryanofsky May 2, 2017 •

edited

Loading