Give CValidationInterface Support for calling notifications on the CScheduler Thread

TheBlueMatt commented at 8:43 pm on April 10, 2017: member

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 https://github.com/TheBlueMatt/bitcoin/commit/4e82e409fbf3473f94dfa8cffe2674e853d59054 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.

fanquake added the label Validation on Apr 10, 2017

laanwj added this to the "Blockers" column in a project

TheBlueMatt force-pushed on Apr 17, 2017

TheBlueMatt commented at 1:54 pm on April 17, 2017: member

Rebased after ##10178 merge.

TheBlueMatt force-pushed on Apr 17, 2017

in src/validationinterface.cpp:36 in e5d0c6662d outdated

30+    // We are not allowed to assume the scheduler only runs in one thread,
31+    // but must ensure all callbacks happen in-order, so we end up creating
32+    // our own queue here :(
33+    CCriticalSection cs_callbacksPending;
34+    std::list<std::function<void (void)>> callbacksPending;
35+    std::atomic<bool> fCallbacksRunning = ATOMIC_VAR_INIT(false);

morcos commented at 6:10 pm on April 24, 2017:

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

TheBlueMatt commented at 10:08 pm on April 24, 2017:

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

sipa commented at 10:25 pm on April 24, 2017:

Could you try

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

?

TheBlueMatt commented at 10:37 pm on April 24, 2017:

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

theuni commented at 7:38 pm on April 26, 2017:

Use braced initialization in a class/struct definition:

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

TheBlueMatt commented at 9:56 pm on April 26, 2017:

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

morcos commented at 6:11 pm on April 24, 2017: member

fast review utACK

laanwj commented at 12:55 pm on April 25, 2017: member

utACK 010f3ae

TheBlueMatt commented at 12:58 pm on April 25, 2017: member

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” notifications@github.com wrote:

utACK e5d0c66 - are you planning on adding tests for this functionality later?

in src/validationinterface.cpp:59 in e5d0c6662d outdated

54+                    break;
55+                }
56+                callback = callbacksPending.front();
57+                callbacksPending.pop_front();
58+            }
59+            callback();

laanwj commented at 1:06 pm on April 25, 2017:

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

TheBlueMatt commented at 2:42 pm on April 25, 2017:

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

laanwj commented at 6:37 am on April 26, 2017:

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.

TheBlueMatt commented at 9:56 pm on April 26, 2017:

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

TheBlueMatt force-pushed on Apr 25, 2017

in src/validationinterface.cpp:97 in 1bcb07f716 outdated

81+CMainSignals::CMainSignals() {
82+    internals = new CMainSignalsInstance();
83+}
84+
85+CMainSignals::~CMainSignals() {
86+    delete internals;

sipa commented at 0:59 am on April 26, 2017:

Use a std::unique_ptr instead?

TheBlueMatt commented at 9:43 pm on April 26, 2017:

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.

sipa commented at 10:00 pm on April 26, 2017:

std::unique_ptr seems to work fine with forward-declared types: https://github.com/sipa/bitcoin/commit/a4ecaad

TheBlueMatt commented at 10:16 pm on April 26, 2017:

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

sipa commented at 10:27 pm on April 26, 2017:

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.

in src/validationinterface.cpp:69 in 1bcb07f716 outdated

64+            }
65+        }
66+    }
67+
68+    void AddToProcessQueue(const std::function<void (void)>& func) {
69+        if (!scheduler)

sipa commented at 1:00 am on April 26, 2017:

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

TheBlueMatt commented at 9:56 pm on April 26, 2017:

assert()ed

in src/validationinterface.cpp:43 in 1bcb07f716 outdated

38+    void ProcessQueue() {
39+        if (fCallbacksRunning.exchange(true)) {
40+            return;
41+        }
42+
43+        while (true) {

sipa commented at 1:03 am on April 26, 2017:

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 commented at 9:56 pm on April 26, 2017:

Done.

sipa commented at 1:08 am on April 26, 2017: member

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.

TheBlueMatt force-pushed on Apr 26, 2017

TheBlueMatt force-pushed on Apr 27, 2017

TheBlueMatt commented at 1:00 am on April 27, 2017: member

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

in src/validationinterface.h:72 in 225f611659 outdated

94+};
95+
96+struct CMainSignalsInstance;
97+class CMainSignals {
98+private:
99+    std::unique_ptr<CMainSignalsInstance> internals;

ryanofsky commented at 4:43 pm on May 2, 2017:

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.

TheBlueMatt commented at 7:56 pm on May 2, 2017:

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

in src/validationinterface.cpp:34 in 4fceec5937 outdated

26@@ -22,6 +27,64 @@ struct CMainSignalsInstance {
27     boost::signals2::signal<void (const CBlockIndex *, const std::shared_ptr<const CBlock>&)> NewPoWValidBlock;
28 
29     CScheduler *scheduler = NULL;
30+
31+    // We are not allowed to assume the scheduler only runs in one thread,
32+    // but must ensure all callbacks happen in-order, so we end up creating
33+    // our own queue here :(
34+    CCriticalSection cs_callbacksPending;

ryanofsky commented at 5:09 pm on May 2, 2017:

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.

TheBlueMatt commented at 8:33 pm on May 2, 2017:

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

ryanofsky commented at 5:13 pm on May 2, 2017: member

utACK 4fceec59372b778f9966485f2a13f97e21b1db4c

in src/validationinterface.cpp:79 in 4fceec5937 outdated

74+        } raiicallbacksrunning(this);
75+
76+        callback();
77+    }
78+
79+    void AddToProcessQueue(const std::function<void (void)>& func) {

ryanofsky commented at 5:19 pm on May 2, 2017:

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.

TheBlueMatt commented at 8:33 pm on May 2, 2017:

Done.

in src/validationinterface.cpp:58 in 4fceec5937 outdated

53+            LOCK(cs_callbacksPending);
54+            if (fCallbacksRunning) return;
55+            if (callbacksPending.empty()) return;
56+            fCallbacksRunning = true;
57+
58+            callback = callbacksPending.front();

ryanofsky commented at 5:21 pm on May 2, 2017:

In commit “Handle more than one CScheduler thread in CValidationInterface”

Could add std::move here.

TheBlueMatt commented at 8:33 pm on May 2, 2017:

Done.

ryanofsky commented at 1:43 pm on May 3, 2017: member

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

TheBlueMatt force-pushed on May 4, 2017

TheBlueMatt commented at 8:51 pm on May 4, 2017: member

Squashed and tweaked commit wording.

ryanofsky commented at 8:51 pm on May 5, 2017: member

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

sipa commented at 7:39 pm on June 3, 2017: member

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?

TheBlueMatt force-pushed on Jun 6, 2017

TheBlueMatt commented at 0:39 am on June 6, 2017: member

Removed C prefix from classes without rebasing.

TheBlueMatt force-pushed on Jun 6, 2017

ryanofsky commented at 3:36 pm on June 6, 2017: member

utACK b04b156d31787884d4c1fd4808b3f1f397d45c04. Only changes were removing “C”, adding “_m”

TheBlueMatt force-pushed on Jun 8, 2017

TheBlueMatt commented at 3:30 pm on June 8, 2017: member

Very minor rebase.

ryanofsky commented at 7:48 pm on June 8, 2017: member

utACK 608fad0e17d47d0943ff5a9425c65a5ea4eeba6f. No changes since last review except fixes to conflicting #include lines.

ryanofsky commented at 5:50 pm on June 12, 2017: member

Status of this PR? It has had 4 utACKs (though @laanwj’s is in strikethrough) as well as comments from @mchrostowski in #10286.

in src/scheduler.h:96 in 608fad0e17 outdated

91+private:
92+    CScheduler *m_pscheduler;
93+
94+    CCriticalSection m_cs_callbacksPending;
95+    std::list<std::function<void (void)>> m_callbacksPending;
96+    bool m_fCallbacksRunning = false;

sipa commented at 8:09 pm on June 12, 2017:

Would you mind changing this to lowercase-only variable names (e.g. m_cs_callbacks_pending, m_callbacks_pending, m_callbacks_running)?

TheBlueMatt commented at 1:12 am on June 21, 2017:

Done.

TheBlueMatt force-pushed on Jun 21, 2017

ryanofsky commented at 3:16 pm on June 21, 2017: member

utACK cdf4cd8f54239bab6d5658ec6eec6c531f34091c. Just naming style changes since last review.

sipa commented at 1:51 am on June 23, 2017: member

@laanwj Does the strikethrough utACK means you still have a concern?

in src/validationinterface.cpp:44 in cdf4cd8f54 outdated

48+    m_internals.reset(new MainSignalsInstance(&scheduler));
49 }
50 
51 void CMainSignals::UnregisterBackgroundSignalScheduler() {
52-    m_internals->m_scheduler = NULL;
53+    m_internals.reset(nullptr);

theuni commented at 10:04 pm on June 27, 2017:

This potentially leaves events in the SingleThreadedSchedulerClient process queue with a dangling pointer to “this”, no? I believe SingleThreadedSchedulerClient needs interrupt/stop functionality.

TheBlueMatt commented at 11:11 pm on June 27, 2017:

Interrupt() will stop the processing of events long before we ever call UnregisterBackgroundSignalScheduler in init/bitcoind.cpp/qt/bitcoin.cpp. There isnt really a great way to solve this, I think, without just making the CScheduler own the thread(/group), but I went ahead and pushed a commit which will just call any remaining callbacks when Unregister is called.

ryanofsky commented at 5:44 pm on July 10, 2017:

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.

laanwj commented at 6:11 am on June 28, 2017: member

@sipa Just that I had a concern at the time, and was too quick to utACK. I should re-review.

in src/validationinterface.h:60 in 010f3aeb2f outdated

81      */
82-    boost::signals2::signal<void (const CBlock&, const CValidationState&)> BlockChecked;
83+    virtual void BlockChecked(const CBlock&, const CValidationState&) {}
84     /** Notifies listeners that a key for mining is required (coinbase) */
85-    boost::signals2::signal<void (std::shared_ptr<CReserveScript>&)> ScriptForMining;
86+    virtual void GetScriptForMining(std::shared_ptr<CReserveScript>&) {};

laanwj commented at 6:22 am on June 28, 2017:

#10683 gets rid of GetScriptForMining in validationinterface, which I think it’s good because it mutates its argument. Doing that in a decoupled thread would be disastrous.

TheBlueMatt commented at 2:13 pm on June 28, 2017:

All of the move-to-background-thread stuff is going to be a slow per-function process. See, eg, #10652 which moves a few functions to the background for net_processing and, of course, #10286 which moves a few to the background for wallet. Agreed that GetScriptForMining should go away and then it wont be a concern :).

laanwj assigned laanwj on Jun 28, 2017

laanwj removed this from the "Blockers" column in a project

theuni commented at 9:22 pm on July 3, 2017: member

utACK (after rebase) now that #10683 is in.

Use TestingSetup to DRY qt rpcnestedtests ff6a834fc3

Make ValidationInterface signals-type-agnostic

(by hiding boost::signals stuff in the .cpp)

This allows us to give it a bit more intelligence as we move
forward, including routing some signals through CScheduler. While
the introduction of a "internals" pointer in the class is pretty
ugly, the fact that we no longer need to include boost/signals
directly from validationinterface.h is very much worth the loss.

3a19fed9db

TheBlueMatt commented at 0:54 am on July 4, 2017: member

Rebased.

TheBlueMatt force-pushed on Jul 4, 2017

TheBlueMatt force-pushed on Jul 7, 2017

TheBlueMatt commented at 3:17 pm on July 7, 2017: member

Redid the shutdown callback-flushing at @morcos’s request - now flushes earlier in the Shutdown() process in a much better location - after all our peers have stopped processing (and thus cant generate callbacks) and before wallet flushing.

Give CMainSignals a reference to the global scheduler

...so that it can run some signals in the background later

cda1429d5b

Add default arg to CScheduler to schedule() a callback now 2fbf2dbe15

Support more than one CScheduler thread for serial clients

This will be used by CValidationInterface soon.

This requires a bit of work as we need to ensure that most of our
callbacks happen in-order (to avoid synchronization issues in
wallet) - we keep our own internal queue and push things onto it,
scheduling a queue-draining function immediately upon new
callbacks.

08096bbbc6

TheBlueMatt force-pushed on Jul 7, 2017

Flush CValidationInterface callbacks prior to destruction

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.

3192975f1d

TheBlueMatt force-pushed on Jul 7, 2017

TheBlueMatt commented at 4:56 pm on July 7, 2017: member

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 commented at 5:17 pm on July 7, 2017: member

utACK 3192975

theuni commented at 11:38 pm on July 7, 2017: member

re-utACK 3192975f1d177aa9f0bbd823c6387cfbfa943610, 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.

in src/validationinterface.cpp:48 in 3192975f1d outdated

43@@ -44,6 +44,10 @@ void CMainSignals::UnregisterBackgroundSignalScheduler() {
44     m_internals.reset(nullptr);
45 }
46 
47+void CMainSignals::FlushBackgroundCallbacks() {
48+    m_internals->m_schedulerClient.EmptyQueue();

ryanofsky commented at 3:35 pm on July 10, 2017:

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.

TheBlueMatt commented at 1:09 am on July 11, 2017:

Added.

in src/init.cpp:254 in cda1429d5b outdated

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

ryanofsky commented at 4:32 pm on July 10, 2017:

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.

TheBlueMatt commented at 1:02 am on July 11, 2017:

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.

in src/scheduler.h:105 in 3192975f1d outdated

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

ryanofsky commented at 5:09 pm on July 10, 2017:

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.

TheBlueMatt commented at 1:09 am on July 11, 2017:

Added the assert instead

ryanofsky commented at 5:50 pm on July 10, 2017: member

utACK 3192975f1d177aa9f0bbd823c6387cfbfa943610. 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.

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

laanwj merged this on Jul 11, 2017

laanwj closed this on Jul 11, 2017

laanwj referenced this in commit 21ed30a314 on Jul 11, 2017

laanwj referenced this in commit 927a1d7d08 on Nov 15, 2017

PastaPastaPasta referenced this in commit 21924ac6c3 on Aug 5, 2019

PastaPastaPasta referenced this in commit f4f7a824bf on Aug 17, 2019

PastaPastaPasta referenced this in commit 07b05b4fc4 on Aug 17, 2019

PastaPastaPasta referenced this in commit 89389b3de6 on Sep 4, 2019

PastaPastaPasta referenced this in commit 40617436c6 on Sep 7, 2019

barrystyle referenced this in commit 1ad51ce13e on Jan 22, 2020

PastaPastaPasta referenced this in commit 61758874b8 on Feb 13, 2020

PastaPastaPasta referenced this in commit 76ed385c03 on Feb 13, 2020

PastaPastaPasta referenced this in commit 3eb80fdcd5 on Feb 29, 2020

PastaPastaPasta referenced this in commit 803530c83d on Feb 29, 2020

random-zebra referenced this in commit 2e510ef586 on Feb 18, 2021

ckti referenced this in commit 38a9ad2b41 on Mar 28, 2021

gades referenced this in commit 1459aa5a72 on Jun 30, 2021

DrahtBot locked this on Sep 8, 2021

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