Utility to run task periodically in a thread #4423
Conversation
There was a problem hiding this comment.
yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@yiwu-arbug has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@yiwu-arbug has updated the pull request. Re-import the pull request |
|
@yiwu-arbug has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| thread.TEST_WaitForRun([&] { mock_env_->set_current_time(i + 1); }); | ||
| } | ||
| // Test function should be exectued exactly kIteraion times. | ||
| ASSERT_EQ(kIteration, count.load()); |
There was a problem hiding this comment.
any idea why Travis test is failing here?
|
@yiwu-arbug has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
There was a problem hiding this comment.
Thanks @yiwu-arbug for this useful class. I've left two minor comments.
util/repeatable_thread.h
Outdated
| // RepeatableThread waiting for timeout. | ||
| bool waiting_; | ||
| // Function ran after caller of TEST_WaitForRun start waiting. | ||
| bool executed_; |
There was a problem hiding this comment.
How about use a counter to denote how many times function_ has executed instead of a simple boolean flag?
There was a problem hiding this comment.
Can you elaborate how that is more useful? I add this boolean to make sure TEST_WaitForRun will wait for at least one run of the function.
There was a problem hiding this comment.
I saw executed_ is set to true each time you execute the function. Therefore I was thinking maybe you can use a counter that is incremented each time the function executes.
There was a problem hiding this comment.
@riversand963 thanks. I changed it to a counter and hopefully it make the code easier to reason about. Mind take a look again when you get a chance?
|
@yiwu-arbug has updated the pull request. Re-import the pull request |
… held when accessing the field
There was a problem hiding this comment.
yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
|
@yiwu-arbug has updated the pull request. Re-import the pull request |
There was a problem hiding this comment.
yiwu-arbug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
| while (!waiting_) { | ||
| cond_var_.Wait(); | ||
| } | ||
| uint64_t prev_count = run_count_; |
There was a problem hiding this comment.
I guess this is minor. Is mutex_ required while calling callback? Maybe should release the lock before and re-acquire later.
There was a problem hiding this comment.
Here I would expect caller use the callback in the same way as in repeatable_thread_test: bumping the time of a MockTimeEnv to trigger next run. In that case whether to unlock doesn't matter much.
There was a problem hiding this comment.
waiting_ will never be true unless mutex_ is released.
Because wait() needs mutex_ to change waiting_ to true.
And wait() changes waiting_ to false before releases mutex_.
Thus RepeatableThreadTest::MockEnvTest will always hang forever.
There was a problem hiding this comment.
line 83 will release mutex, and the waiting_ check at line 59 will then pass. The MockEnvTest stops timer at this point so the loop of line 82-87 cannot break before line 59 passes.
There was a problem hiding this comment.
Sorry, I didn't get it.
How does line 83 release mutex_ ?
mutex_ is locked on line 75, and I think it can only be released at the end of wait(), i.e. line 93.
Am I right?
There was a problem hiding this comment.
@solicomo you miss the fact that cond_var.wait() implicitly releases mutex before go to sleep and re-acquire the mutex after come back from sleep.
There was a problem hiding this comment.
@yiwu-arbug Thanks for your explanation.
I run repeatable_thread_test on a Windows machine, all tests passed.
But on my Mac, RepeatableThreadTest::MockEnvTest always hang forever.
I investigated it today and found:
TEST_WaitForRun() always hang on line 60 while wait() gets trapped into an endless loop on line 82-87. Because env_->NowMicros() always returns 0.
I guess the reason is:
wait()thread always gets the opportunity to run.- we are using
MockEnv, ifTEST_WaitForRun()has no chance to callmock_env_->set_current_time(i), time will never elapse, thus the loop on line 82-87 will be infinite.
To fix it, we can use default env instead of MockEnv.
There was a problem hiding this comment.
@solicomo Thank you for your feedback. Very appreciate it. I come up with the MockEnvTest and TEST_WaitForRun() in the hope someone use RepeatableThread can have a way to mock time functions and have unit test that's not flaky. The TimedTest is intended to be an example of the same test with default env. Let me try to find a way to avoid the TEST_WaitForRun() being starve.
Summary:
Introduce
RepeatableThreadutility to run task periodically in a separate thread. It is basically the same as the the same class in fbcode, and in addition provide a helper method to let tests mock time and trigger execution one at a time.We can use this class to replace
TimerQueuein #4382 andBlobDB.Test Plan:
New tests.