/bitcoin

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

Merged
merged 7 commits into from Jul 11, 2017
+274 −73

Conversation

Reviewers

@laanwj laanwj

@theuni theuni

@sipa sipa

@morcos morcos

@ryanofsky ryanofsky

Assignees

@laanwj laanwj

Labels
Validation
Projects
None yet
Milestone
No milestone
7 participants
@TheBlueMatt @morcos @laanwj @ryanofsky @sipa @theuni @fanquake
@TheBlueMatt
Contributor

TheBlueMatt commented Apr 10, 2017

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

fanquake added the Validation label Apr 10, 2017

laanwj added to Blockers in High-priority for review Apr 13, 2017

@TheBlueMatt
Contributor

TheBlueMatt commented Apr 17, 2017

Rebased after ##10178 merge.
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);
@morcos

morcos Apr 24, 2017

Contributor

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

TheBlueMatt Apr 24, 2017

Contributor

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

@sipa

sipa Apr 24, 2017

Owner

Could you try

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

?

@TheBlueMatt

TheBlueMatt Apr 24, 2017

Contributor

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

@theuni

theuni Apr 26, 2017

Member

Use braced initialization in a class/struct definition:

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

TheBlueMatt Apr 26, 2017

Contributor

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

@morcos
Contributor

morcos commented Apr 24, 2017

fast review utACK
@laanwj
Owner

laanwj commented Apr 25, 2017 • edited

utACK 010f3ae
@TheBlueMatt
Contributor

TheBlueMatt commented Apr 25, 2017

src/validationinterface.cpp
+ callback = callbacksPending.front();
+ callbacksPending.pop_front();
+ }
+ callback();
@laanwj

laanwj Apr 25, 2017

Owner

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

@TheBlueMatt

TheBlueMatt Apr 25, 2017

Contributor

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

@laanwj

laanwj Apr 26, 2017

Owner

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

TheBlueMatt Apr 26, 2017

Contributor

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

@sipa

sipa reviewed Apr 26, 2017
View changes

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.

src/validationinterface.cpp
+ return;
+ }
+
+ while (true) {
@sipa

sipa Apr 26, 2017

Owner

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

TheBlueMatt Apr 26, 2017

Contributor

Done.

src/validationinterface.cpp
+ }
+
+ void AddToProcessQueue(const std::function<void (void)>& func) {
+ if (!scheduler)
@sipa

sipa Apr 26, 2017

Owner

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

@TheBlueMatt

TheBlueMatt Apr 26, 2017

Contributor

assert()ed

src/validationinterface.cpp
+}
+
+CMainSignals::~CMainSignals() {
+ delete internals;
@sipa

sipa Apr 26, 2017

Owner

Use a std::unique_ptr instead?

@TheBlueMatt

TheBlueMatt Apr 26, 2017

Contributor

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

sipa Apr 26, 2017

Owner

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

@TheBlueMatt

TheBlueMatt Apr 26, 2017

Contributor

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

@sipa

sipa Apr 26, 2017

Owner

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.

TheBlueMatt referenced this pull request Apr 27, 2017

Open

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

@TheBlueMatt
Contributor

TheBlueMatt commented Apr 27, 2017

@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.
src/validationinterface.h
+struct CMainSignalsInstance;
+class CMainSignals {
+private:
+ std::unique_ptr<CMainSignalsInstance> internals;
@ryanofsky

ryanofsky May 2, 2017

Contributor

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

TheBlueMatt May 2, 2017

Contributor

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

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;
@ryanofsky

ryanofsky May 2, 2017

Contributor

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

TheBlueMatt May 2, 2017

Contributor

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

src/validationinterface.cpp
+ callback();
+ }
+
+ void AddToProcessQueue(const std::function<void (void)>& func) {
@ryanofsky

ryanofsky May 2, 2017 • edited

Contributor

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

TheBlueMatt May 2, 2017

Contributor

Done.

src/validationinterface.cpp
+ if (callbacksPending.empty()) return;
+ fCallbacksRunning = true;
+
+ callback = callbacksPending.front();
@ryanofsky

ryanofsky May 2, 2017

Contributor

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

Could add std::move here.

@TheBlueMatt

TheBlueMatt May 2, 2017

Contributor

Done.

@ryanofsky
Contributor

ryanofsky commented May 3, 2017

utACK 643a988. I like the new ProcessQueue location and std::move additions.
@TheBlueMatt
Contributor

TheBlueMatt commented May 4, 2017

Squashed and tweaked commit wording.
@ryanofsky
Contributor

ryanofsky commented May 5, 2017

utACK 830c26b. Only change aside from the history squashing is switching to the schedule() default arg.
@sipa
Owner

sipa commented Jun 3, 2017

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
Contributor

TheBlueMatt commented Jun 6, 2017

Removed C prefix from classes without rebasing.
@ryanofsky

ryanofsky reviewed Jun 6, 2017
View changes

utACK b04b156. Only changes were removing "C", adding "_m"

@TheBlueMatt
Contributor

TheBlueMatt commented Jun 8, 2017

Very minor rebase.
@ryanofsky

ryanofsky reviewed Jun 8, 2017
View changes

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

@ryanofsky
Contributor

ryanofsky commented Jun 12, 2017

Status of this PR? It has had 4 utACKs (though @laanwj's is in strikethrough) as well as comments from @mchrostowski in #10286.
src/scheduler.h
+
+ CCriticalSection m_cs_callbacksPending;
+ std::list<std::function<void (void)>> m_callbacksPending;
+ bool m_fCallbacksRunning = false;
@sipa

sipa Jun 12, 2017

Owner

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

@TheBlueMatt

TheBlueMatt Jun 21, 2017

Contributor

Done.

@ryanofsky

ryanofsky reviewed Jun 21, 2017
View changes

utACK cdf4cd8. Just naming style changes since last review.

TheBlueMatt referenced this pull request Jun 22, 2017

Open

Small step towards demangling cs_main from CNodeState #10652

@sipa
Owner

sipa commented Jun 23, 2017

@laanwj Does the strikethrough utACK means you still have a concern?
src/validationinterface.cpp
}
void CMainSignals::UnregisterBackgroundSignalScheduler() {
- m_internals->m_scheduler = NULL;
+ m_internals.reset(nullptr);
@theuni

theuni Jun 27, 2017

Member

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

@TheBlueMatt

TheBlueMatt Jun 27, 2017

Contributor

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

ryanofsky Jul 10, 2017

Contributor

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
Owner

laanwj commented Jun 28, 2017

@sipa Just that I had a concern at the time, and was too quick to utACK. I should re-review.
src/validationinterface.h
/** Notifies listeners that a key for mining is required (coinbase) */
- boost::signals2::signal<void (std::shared_ptr<CReserveScript>&)> ScriptForMining;
+ virtual void GetScriptForMining(std::shared_ptr<CReserveScript>&) {};
@laanwj

laanwj Jun 28, 2017

Owner

#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

TheBlueMatt Jun 28, 2017

Contributor

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 self-assigned this Jun 28, 2017

laanwj removed from Blockers in High-priority for review Jun 29, 2017

@theuni
Member

theuni commented Jul 3, 2017

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

TheBlueMatt added some commits Apr 27, 2017

@TheBlueMatt TheBlueMatt Use TestingSetup to DRY qt rpcnestedtests ff6a834
@TheBlueMatt TheBlueMatt 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.
 3a19fed
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 4, 2017

Rebased.
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 7, 2017

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.

TheBlueMatt added some commits Jan 19, 2017

@TheBlueMatt TheBlueMatt Give CMainSignals a reference to the global scheduler 
...so that it can run some signals in the background later
 cda1429
@TheBlueMatt TheBlueMatt Add default arg to CScheduler to schedule() a callback now 2fbf2db
@TheBlueMatt TheBlueMatt 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.
 08096bb
@TheBlueMatt TheBlueMatt 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.
3192975
@TheBlueMatt
Contributor

TheBlueMatt commented Jul 7, 2017

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
Contributor

morcos commented Jul 7, 2017

utACK 3192975
@theuni
Member

theuni commented Jul 7, 2017

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

ryanofsky reviewed Jul 10, 2017
View changes

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.

src/validationinterface.cpp
@@ -44,6 +44,10 @@ void CMainSignals::UnregisterBackgroundSignalScheduler() {
m_internals.reset(nullptr);
}
+void CMainSignals::FlushBackgroundCallbacks() {
+ m_internals->m_schedulerClient.EmptyQueue();
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

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

TheBlueMatt Jul 11, 2017

Contributor

Added.

src/validationinterface.cpp
}
void CMainSignals::UnregisterBackgroundSignalScheduler() {
- m_internals->m_scheduler = NULL;
+ m_internals.reset(nullptr);
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

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.

src/init.cpp
@@ -251,6 +251,7 @@ void Shutdown()
}
#endif
UnregisterAllValidationInterfaces();
+ GetMainSignals().UnregisterBackgroundSignalScheduler();
@ryanofsky

ryanofsky Jul 10, 2017

Contributor

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

TheBlueMatt Jul 11, 2017

Contributor

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.

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

ryanofsky Jul 10, 2017

Contributor

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

TheBlueMatt Jul 11, 2017

Contributor

Added the assert instead

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

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@laanwj laanwj added a commit that referenced this pull request Jul 11, 2017

@laanwj laanwj Merge #10179: Give CValidationInterface Support for calling notificat… 
…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
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 1E4AED62986CD25D Learn about signing commits
21ed30a

This was referenced Jul 11, 2017

Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment