GuardDog: Test: Fix race in mocked time source. #719
Conversation
The guard dog test made additional EXPECT_CALLs to update the time the mocked time source should return after the mocked object was passed to the GD thread. Doing additional EXPECT_CALLs in this way is NOT thread safe and resulted in 0.5-1% of tests failing when many were executed in parallel. The solution is to make the expect call action be to invoke a lambda that shares access to a safe atomic updated in the main test thread.
test/server/guarddog_impl_test.cc
Outdated
|
|
||
| /** | ||
| * This does everything but the final forceCheckForTest() that should cause | ||
| * death for the single kill case. | ||
| */ | ||
| void SetupForDeath() { | ||
| InSequence s; | ||
| EXPECT_CALL(time_source_, currentSystemTime()).WillRepeatedly(testing::Return(time_point_)); | ||
| EXPECT_CALL(time_source_, currentSystemTime()) |
There was a problem hiding this comment.
nit: EXPECT_CALL() with WillRepeatedly is an anti-pattern in that it doesn't actually expect anything since 0 times will pass. I would make this ON_CALL(...).WillByDefault(...). (Same for all below).
There was a problem hiding this comment.
You could also likely move this into fixture constructor to avoid duplication.
There was a problem hiding this comment.
Good point, I'll fix this.
test/server/guarddog_impl_test.cc
Outdated
| : config_kill_(1000, 1000, 100, 1000), config_multikill_(1000, 1000, 1000, 500) {} | ||
|
|
||
| void SetupMockTimeSource() { | ||
| ON_CALL(time_source_, currentSystemTime()) |
There was a problem hiding this comment.
Just put this directly in constructor, then you don't have to call from each test.
test/server/guarddog_impl_test.cc
Outdated
| }; | ||
|
|
||
| TEST_F(GuardDogMissTest, MissTest) { | ||
| // This test checks the actual collected statistics after doing some timer | ||
| // advances that should and shouldn't increment the counters. | ||
| EXPECT_CALL(time_source_, currentSystemTime()).WillRepeatedly(testing::Return(time_point_)); | ||
| ON_CALL(time_source_, currentSystemTime()) |
There was a problem hiding this comment.
This should go in constructor of GuardDogMissTest
There was a problem hiding this comment.
LGTM. @mattklein123 SIGILL due to corrupted vtable or however gmock is doing its mocking?
|
That's my guess. Consider this sequence:
If step 2 begins executing before the loop reaches the call to currentSystemTime() then it will be following a wild address when calling currentSystemTime(). |
|
Beyond this change I also think that #717 was involved in odd crashes. |
* Strip out "spiffe://" in the identity * Addressed some review comments. * Addressed review comments.
* Revert "Strip out "spiffe://" in the identity (envoyproxy#719)" This reverts commit 99a482f. * Revert "Revert "Remove -release in filename when doing release build of proxy (envoyproxy#704)" (envoyproxy#723)" This reverts commit 13669ce. * Revert "Not to send legacy quota for v2 config. (envoyproxy#722)" This reverts commit aaf25ca.
Like _free_base, _free_dbg is called by CRT internal functions or operator delete in Debug mode. This closes envoyproxy#719 and closes envoyproxy#894. [alkondratenko@gmail.com: trivial formatting fixes] [alkondratenko@gmail.com: build free_dbg even in release builds]
The guard dog test made additional EXPECT_CALLs to update the time the
mocked time source should return after the mocked object was passed to
the GD thread. Doing additional EXPECT_CALLs in this way is NOT thread
safe and resulted in 0.5-1% of tests failing when many were executed in
parallel.
The solution is to make the expect call action be to invoke a lambda
that shares access to a safe atomic updated in the main test thread.