/bitcoin

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

CCheckQueue Unit Tests #9497

Merged
merged 2 commits into from Mar 14, 2017
+455 −10

Conversation

Reviewers

@kallewoof kallewoof

@instagibbs instagibbs

@ryanofsky ryanofsky

Assignees
No one assigned
Labels
Tests
Projects
None yet
Milestone
No milestone
6 participants
@JeremyRubin @laanwj @kallewoof @instagibbs @ryanofsky @fanquake
@JeremyRubin
Contributor

JeremyRubin commented Jan 9, 2017

This PR builds on #9495 to unit test the CCheckQueue for correctness.

The cases covered in these tests are:

  1. Standard usage
  2. Failing checks are caught
  3. Prior blocks failing don't interfere with future blocks
  4. No Memory leakage (all check destructors are called before new blocks allowed, memory is freed).
  5. Thread Safety
src/test/checkqueue_tests.cpp
@@ -0,0 +1,395 @@
+// Copyright (c) 2012-2015 The Bitcoin Core developers
@instagibbs

instagibbs Jan 9, 2017

Member

couple of years out of date :)

@JeremyRubin

JeremyRubin Jan 9, 2017

Contributor

will fix!

@fanquake fanquake added the Tests label Jan 9, 2017

@JeremyRubin
Contributor

JeremyRubin commented Jan 9, 2017

Sorry for the line noise; the earlier build error should be addressed now.
@ryanofsky

ryanofsky reviewed Jan 11, 2017
View changes

Overall great and very clever tests (thread safety one was fun to figure out). Added a bunch of minor comments. The only two comments I would really urge you to consider are the ones on the Memory and FrozenCleanup tests. I think it would be good to check same conditions without allocating large chunks of memory or doing 1-second busy loops so these tests can be more efficient and more reliable.

src/test/checkqueue_tests.cpp
+#include <unordered_set>
+#include <memory>
+#include "random.h"
+BOOST_FIXTURE_TEST_SUITE(checkqueue_tests, TestingSetup)
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Maybe add a comment noting BasicTestingSetup can't be used because it doesn't set nScriptCheckThreads.

src/test/checkqueue_tests.cpp
+ */
+void Correct_Queue_range(std::vector<size_t> range)
+{
+ auto small_queue = std::shared_ptr<Correct_Queue>(new Correct_Queue {128});
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Maybe declare 128 and any other common parameters as constants above.

src/test/checkqueue_tests.cpp
+ */
+void Correct_Queue_range(std::vector<size_t> range)
+{
+ auto small_queue = std::shared_ptr<Correct_Queue>(new Correct_Queue {128});
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Why is small_queue a shared pointer, not a unique pointer or just plain stack variable? Maybe add a comment explaining. Also, you could probably use make_shared if it does need to be a shared pointer.

src/test/checkqueue_tests.cpp
+ auto small_queue = std::shared_ptr<Correct_Queue>(new Correct_Queue {128});
+ boost::thread_group tg;
+ for (auto x = 0; x < nScriptCheckThreads; ++x) {
+ tg.create_thread([=]{small_queue->Thread();});
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Replacing [=] with [&] might allow small_queue not to be a shared_ptr.

src/test/checkqueue_tests.cpp
+ while (total) {
+ size_t r = GetRand(10);
+ std::vector<FakeCheckCheckCompletion> vChecks;
+ for (size_t k = 0; k < r && total; k++) {
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Maybe replace the loop with vCheck.resize(min(total, r)).

src/test/checkqueue_tests.cpp
+ }
+ result[end_fails ? 0 : 1] = control.Wait();
+ }
+ BOOST_CHECK(!result[0]);
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Would seem more direct to just BOOST_CHECK the control.Wait() call instead of putting the results in an intermediate array. This way is ok too, though.

src/test/checkqueue_tests.cpp
+ bool r = true;
+ for (size_t i = 0; i < COUNT; ++i)
+ r = r && UniqueCheck::results.count(i) == 1;
+ BOOST_REQUIRE(r);
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Maybe also check that UniqueCheck::results.size == COUNT.

src/test/checkqueue_tests.cpp
+
+// Test that blocks which might allocate lots of memory free their memory agressively.
+//
+// This test attempts to catch a pathalogical case where by lazily freeing
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

pathological (spelling)

src/test/checkqueue_tests.cpp
+// checks might mean leaving a check un-swapped out, and decreasing by 1 each
+// time could leave the data hanging across a sequence of blocks.
+//
+// This test (failing) is dependent on not being able to handle
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

Instead of having the test be nondeterministic in this way, would anything be lost if you had the MemoryCheck constructor increment a static counter when passed a true arg, and the MemoryCheck destructor decrement the counter if the object was constructed with a true arg. Then you could detect the error case explicitly by checking the counter, and not have to allocate big chunks of memory.

src/test/checkqueue_tests.cpp
+ //
+ // Note that this cannot cause a spurious failure, only could mean that
+ // the test doesn't actually end up checking that control waited.
+ MilliSleep(1000);
@ryanofsky

ryanofsky Jan 11, 2017

Contributor

I think you could easily make this test deterministic as well, eliminating the long sleep and the while (frozen) busy loops. Would just need to have ~FrozenCleanupCheck increment a counter and signal a conditional variable so you could wait here for enough jobs to be frozen, and then do the boost check. Then this could signal another condition variable to unfreeze the jobs. I think it would be worth changing this to make the test more efficient and reliable.

@ryanofsky

ryanofsky reviewed Jan 13, 2017
View changes

utACK d44af13. Left one minor comment, feel free to ignore.

src/test/checkqueue_tests.cpp
- while (!done && !fails2) {
- fails2 = queue->ControlMutex.try_lock();
+ std::mutex m;
+ bool has_lock {false};
@ryanofsky

ryanofsky Jan 13, 2017

Contributor

It might be clearer to replace all these bools with an enum like { STARTED, TRY_LOCK, TRY_LOCK_DONE, DONE, DONE_ACK }.

@JeremyRubin

JeremyRubin Jan 16, 2017

Contributor

Going to ignore this, I don't think it makes it more clear (to me, it's easier to debug several variables). If someone disagrees strongly, will change.

@laanwj
Owner

laanwj commented Jan 19, 2017

ACK, needs squashing
@JeremyRubin
Contributor

JeremyRubin commented Jan 19, 2017

Squashed!
@JeremyRubin
Contributor

JeremyRubin commented Jan 20, 2017

Rebased to be on top of #9495.
src/test/checkqueue_tests.cpp
+ control.Add(vChecks);
+ }
+ }
+ BOOST_REQUIRE(MemoryCheck::fake_allocated_memory == 0);
@kallewoof

kallewoof Jan 30, 2017 •
Edited 1 time

Member

The MemoryCheck struct destructor does not --, so this should not be == 0 unless no MemoryCheck constructors are ever called.

@JeremyRubin

JeremyRubin Feb 16, 2017

Contributor

Yes, it was the latter. The for loop never made anything (i = 9999; i<9999). Will fix :)

src/test/checkqueue_tests.cpp
+ };
+ ~MemoryCheck(){
+ if (b) {
+ fake_allocated_memory += 1;
@kallewoof

kallewoof Jan 30, 2017

Member

fake_allocated_memory -= 1

src/test/checkqueue_tests.cpp
+BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure)
+{
+ auto fail_queue = std::unique_ptr<Failing_Queue>(new Failing_Queue {QUEUE_BATCH_SIZE});
+ std::array<FailingCheck, 100> checks;
@kallewoof

kallewoof Jan 30, 2017

Member

Unused variable.

src/test/checkqueue_tests.cpp
+ // Wait until the queue has finished all jobs and frozen
+ FrozenCleanupCheck::cv.wait(l, [](){return FrozenCleanupCheck::nFrozen == 1;});
+ // Try to get control of the queue a bunch of times
+ for (auto x = 0; x < 100; ++x) {
@kallewoof

kallewoof Jan 30, 2017

Member

Doing !fails && x < 100 here and simply fails = queue->ControlMutex.try_lock(); would break iteration on first fail rather than iterate over all 100 (e.g. if first try_lock() fails).

@JeremyRubin

JeremyRubin Feb 16, 2017

Contributor

sure..

src/test/checkqueue_tests.cpp
+ // Wait for thread to get the lock
+ cv.wait(l, [&](){return has_lock;});
+ bool fails = false;
+ for (auto x = 0; x < 100; ++x) {
@kallewoof

kallewoof Jan 30, 2017

Member

Same here with !fails && x < 100 as above.

@JeremyRubin
Contributor

JeremyRubin commented Feb 16, 2017

Fixed the issues that @kallewoof raised, and squashed.

Unsquashed preserved here https://github.com/JeremyRubin/bitcoin/tree/checkqueue-tests-unsquashed.
@kallewoof
Member

kallewoof commented Feb 25, 2017 •
Edited 1 time

utACK 96c7f2c

I'm a bit concerned about non-deterministic behavior in tests as this tends to be a pain when you do run into a problem. Or is this fixed seed / PRNG so that the numbers are always the same each time? (for GetRand())
@JeremyRubin
Contributor

JeremyRubin commented Feb 25, 2017

I could make them deterministic if that's desirable, but realistically these tests are already non-deterministic by virtue of being multithreaded. None of the uses of GetRand are particularly dangerous here, although perhaps they area a little slower than could be.
@kallewoof
Member

kallewoof commented Feb 25, 2017

I think that would be desirable, even if the multithreading makes it not 100%.
@kallewoof
Member

kallewoof commented Feb 26, 2017

ACK 96c7f2c

@JeremyRubin JeremyRubin referenced this pull request Mar 7, 2017

Open

Lock-Free CheckQueue #9938

@laanwj laanwj merged commit 96c7f2c into bitcoin:master Mar 14, 2017
1 check passed

1 check passed

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

laanwj added a commit that referenced this pull request Mar 14, 2017

@laanwj
Merge #9497: CCheckQueue Unit Tests 
96c7f2c Add CheckQueue Tests (Jeremy Rubin)
e207342 Fix CCheckQueue IsIdle (potential) race condition and remove dangerous constructors. (Jeremy Rubin)

Tree-SHA512: 5989743ad0f8b08998335e7ca9256e168fa319053f91b9dece9dbb134885bef7753b567b591acc7135785f23d19799ed7e6375917f59fe0178d389e961633d62
This commit was signed with a verified signature.
@laanwj laanwj Wladimir J. van der Laan
GPG key ID: 74810B012346C9A6 Learn about signing commits
2c781fb

This was referenced Mar 15, 2017

Merged
Closed

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

@luke-jr
Merge #9497 into 0.14.2_fixes
f1c348e

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

@luke-jr
Merge #9497 via remote-tracking branch 'origin-pull/9497/head' into 0… 
….14.2_fixes
b71b605

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Jun 5, 2017

@luke-jr
Merge #9497 via remote-tracking branch 'origin-pull/9497/head' into 0… 
….14.2_fixes
1b864c9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment