search

dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

Remove waiter_test #973

Closed frolosofsky closed 5 years ago

frolosofsky commented 5 years ago
  1. Remove lock before notify_all as it is redundant according to https://en.cppreference.com/w/cpp/thread/condition_variable/notify_all
  2. Remove waiter_test as it's a trivial wrapper on conditional variable and having this test means checking C++ stdlib. And this test was quite flaky when being run in heavy-parallel surrounding.

Fixes #933.

scravy commented 5 years ago

utACK cfe3839413639ad83ba95dd238b4afaa71671790

frolosofsky commented 5 years ago

It failed again right in this PR.

Entering test module "Unit-e Test Suite"
test/waiter_tests.cpp(14): Entering test suite "waiter_tests"
test/waiter_tests.cpp(29): Entering test case "wait_and_wake_test"
test/waiter_tests.cpp(22): fatal error: in "waiter_tests/wait_and_wake_test": critical check now < wait_until has failed

Replaced yield with sleep_for(millisecond) as first one produces 100% load on the core, maybe it affects tests being run with hight parallelism. I still don't see any deadlock in the code, in fact it has simplified to trivial condition variable wrapper. This fact makes me think we don't need this test at all, as it is checking C++ stdlib rather custom logic.

cmihai commented 5 years ago

I agree that proposer::Waiter is a trivial wrapper over std::condition_variable, and would be ok with removing the test.

Edit: ....and it failed again, even with the sleep. Either there's something wrong with the C++ stdlib on Windows, or there's a deadlock in the test we don't see.

frolosofsky commented 5 years ago

Ok this guy flaked again but differently. I think, it's nice to delete them completely. Reasoning in PR description and https://github.com/dtr-org/unit-e/pull/973#issuecomment-483195256.

@scravy @cmihai @thothd re-review it please.

frolosofsky commented 5 years ago

Edit: ....and it failed again, even with the sleep. Either there's something wrong with the C++ stdlib on Windows, or there's a deadlock in the test we don't see.

Last time it failed differently: in one of the test scenarios where we expected the thread to wake up in 10 seconds. The fact that it didn't fail in 60 seconds assertion tells us that it has been actually woken up but after more than 10 seconds.

So, it was neither a bug in stdlib nor dead lock. In this particular time at least.

There is no "windows C++ stdlib", we use mingw compiler and I guess it uses gcc C++ stdlib. Windows doesn't ship C++ stdlib at all AFAIK.