This accumulates a bunch of cleanup that was long overdue, but I haven't yet gotten around to address. Specifically, but not limited to:
Please refer to the individual commits for more details.
No longer needed after commit d61f2bb076d8f
The common setup is included in virtually all tests, so it should be
as slim as possible.
Concept ACK
77 | - // done servicing whatever task they're currently servicing (drain=false) 78 | + // Tell any threads running serviceQueue to stop as soon as they are 79 | + // done servicing whatever task they are currently servicing (drain=false) 80 | // or when there is no work left to be done (drain=true) 81 | - void stop(bool drain=false); 82 | + void stop(bool drain);
Might be better to have something like:
// Tell any threads running serviceQueue to stop as soon as they are
// done servicing whatever task they are currently servicing.
void stop_immediately();
// Tell any threads running serviceQueue to stop as soon as
// there is no work left to be done.
void stop_when_drained();
Sure, will do that
Done
60 | @@ -62,8 +61,6 @@ class CScheduler 61 | */ 62 | void MockForward(std::chrono::seconds delta_seconds); 63 | 64 | - // To keep things as simple as possible, there is no unschedule.
Hmm, I kind-of think that comment's helpful.
This should be clear from our general policy that everything should be as minimal as possible and that unused features should not be implemented.
If the comment is brought back, I'd prefer the alternative version:
// To keep things as simple as possible, there is no unschedule and the scheduler can not schedule events on your electronic barista
Concept ACK
After commit d0ebd93 the scheduler itself no longer cares if the
serviceQueue is run in a std::thread or boost::thread. Change the
documentation to std::thread because we switched to C++11.
39 | + * Simple class for background tasks that should be run 40 | + * periodically or once "after a while" 41 | + * 42 | + * Usage: 43 | + * 44 | + * CScheduler* s = new CScheduler();
If you're changing this, perhaps make it a unique_ptr to indicate unique ownership?
Not sure how to preserve this part of the comment then:
delete s; // Must be done after thread is interrupted/joined.
If this is changed, I'd rather just
CScheduler s{};
s.scheduleFromNow(doSomething, std::chrono::milliseconds{11}); // Assuming a: void doSomething() { }
s.scheduleFromNow([=] { this->func(argument); }, std::chrono::milliseconds{3});
std::thread t{[&] { s.serviceQueue(); }};
... then at program shutdown, make sure to call stop() to clean up the thread(s) running serviceQueue:
s.stop();
t.join();
That looks reasonable to me, although I don't think it's that important. If you don't think it matters, leave it as is to avoid bikeshedding.
However, I do think that you should s/stop()/StopImmediately()/ in this comment now that you're removing the stop() function.
Left as is for now.
80 | + { 81 | + WITH_LOCK(newTaskMutex, stopRequested = true); 82 | + newTaskScheduled.notify_all(); 83 | + } 84 | + /** Tell any threads running serviceQueue to stop when there is no work left to be done */ 85 | + void StopWhenDrained()
This is a really good change. Explicitly naming these functions makes it much clearer at the call site.
Do you think a follow-up should remove the StopWhenDrained() function entirely? It's only used in tests.
Done
94 | - std::chrono::system_clock::time_point &last) const; 95 | + /** 96 | + * Returns number of tasks waiting to be serviced, 97 | + * and first and last task times 98 | + */ 99 | + size_t getQueueInfo(std::chrono::system_clock::time_point& first, std::chrono::system_clock::time_point& last) const;
I find it more readable to to have the arguments split over multiple lines, though I guess that means I should open a PR to remove AllowAllParametersOfDeclarationOnNextLine: false from .clang-format?
In this case splitting over multiple lines is allowed because BinPack... is already set to false. However, unless AlignAfterOpenBracket is changed to Align I find it less readable.
In any case changing .clang-format should be done in a separate pr, and I might object changing AlignAfterOpenBracket to Align because it wastes so much whitespace.
thx fixed
Code review ACK fae1ef5e7
Removed some more code as requested by @jnewbery
This helps understanding the code at the call site without having to
look up the name of the argument or the default value.
Reverted back to the previous version for a smaller diff
reACK fa197f1d1119346b8980cf1a8b34ceee07f30a6a
only change is renaming StopImmediately() to stop() (presumably so StopWhenDrained() can be removed in a follow-up PR.
Reverted back to the previous version for a smaller diff
Good. My original suggestion was to remove StopWhenDrained() in a follow-up. No need to do everything in one PR.
Can easily be reviewed with the git options
--ignore-all-space --word-diff-regex=. -U0
Force pushed last commit, but should be trivial to re-review with git range-diff bitcoin-core/master --word-diff-regex=. A B
utACK fa8337fcdbcab8368d5bf111c4b2e5acf25e6e22