semaphore_test.cc: add TimedWaitSpuriousWakeupLinux #1037

dconeybe · 2022-07-20T15:51:15Z

Add a unit test for the Semaphore::TimedWait() spurious wakeup fix in #1021.

…s with the TimedWait() fix in #1021

github-actions · 2022-07-20T17:08:44Z

❌ Integration test FAILED

Requested by @dconeybe on commit 69b870d
Last updated: Tue Jul 26 13:40 PDT 2022
View integration test log & download artifacts

Failures	Configs
firestore	[TEST] [FAILURE] [iOS] [macos] [1/2 ios_device: simulator_target] (1 failed tests) NumericTransformsTest.CreateDocumentWithIncrement [TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/2 android_device: android_target] (1 failed tests) ServerTimestampTest.TestServerTimestampsUsesPreviousValueFromLocalMutation
gma	[TEST] [FLAKINESS] [iOS] [macos] [1/2 ios_device: ios_target] (1 failed tests) FirebaseGmaTest.TestRewardedAdStress
storage	[TEST] [FLAKINESS] [iOS] [macos] [1/2 ios_device: ios_target] (1 failed tests) FirebaseStorageTest.TestWriteAndReadCustomContentType

Add flaky tests to go/fpl-cpp-flake-tracker

…eads library.

… it is out-of-scope for this PR

jonsimantov · 2022-07-21T18:33:06Z

app/tests/semaphore_test.cc

@@ -76,6 +88,43 @@ TEST(SemaphoreTest, TimedWait) {
      0.20 * firebase::internal::kMillisecondsPerSecond);
 }

+#if SEMAPHORE_LINUX


WDYT about just using #ifdef FIREBASE_ANDROID || FIREBASE_LINUX here and above? To me that's clearer, even though it's not the same as the check in semaphore.h.

jonsimantov · 2022-07-21T18:39:32Z

app/tests/semaphore_test.cc

+
+  // Make sure that Semaphore::TimedWait() blocked for the entire timeout, and,
+  // specifically, did NOT return early as a result of the SIGUSR1 interruption.
+  ASSERT_LT(labs((finish_ms - start_ms) -


Can we just use EXPECT_NEAR here?

Yes, EXPECT_NEAR works perfectly. Done.

jonsimantov · 2022-07-21T18:40:21Z

app/tests/semaphore_test.cc

+  // Call Semaphore::TimedWait() and keep track of how long it blocks for.
+  firebase::Semaphore sem(0);
+  int64_t start_ms = firebase::internal::GetTimestamp();
+  EXPECT_FALSE(sem.TimedWait(2 * firebase::internal::kMillisecondsPerSecond));


2 * firebase::internal::kMillisecondsPerSecond should be in a constant in this function, for clarity

jonsimantov · 2022-07-21T18:43:44Z

app/tests/semaphore_test.cc

+TEST(SemaphoreTest, TimedWaitSpuriousWakeupLinux) {
+  // Register a dummy signal handler for SIGUSR1; otherwise, sending SIGUSR1
+  // later on will crash the application.
+  signal(SIGUSR1, [](int signum) -> void {});


Does this actually need the trailing return type syntax?

No it does not. I've removed the trailing return.

jonsimantov · 2022-07-21T18:44:39Z

app/tests/semaphore_test.cc

+  // specifically, did NOT return early as a result of the SIGUSR1 interruption.
+  ASSERT_LT(labs((finish_ms - start_ms) -
+                 (2 * firebase::internal::kMillisecondsPerSecond)),
+            0.20 * firebase::internal::kMillisecondsPerSecond);


The allowed delta should probably also be in a constant

jonsimantov

Approved with one nit

jonsimantov · 2022-07-25T21:33:26Z

app/tests/semaphore_test.cc

+  // Call Semaphore::TimedWait() and keep track of how long it blocks for.
+  firebase::Semaphore sem(0);
+  auto start_ms = firebase::internal::GetTimestamp();
+  const int timed_wait_timeout = 2 * firebase::internal::kMillisecondsPerSecond;


I think the constants should probably follow the kConstantNaming syntax

dconeybe · 2022-07-26T04:14:15Z

@jonsimantov Please re-approve, as your previous approval was dismissed as being "stale".

dconeybe added 3 commits July 19, 2022 22:09

semaphore_test.cc: re-enable MultithreadedStressTest, which now passe…

b9da066

…s with the TimedWait() fix in #1021

semaphore_test.cc: TimedWaitSpuriousWakeupLinux added

50d9f7d

semaphore_test.cc: add comments

b58cec2

dconeybe added the skip-release-notes Skip release notes check label Jul 20, 2022

dconeybe self-assigned this Jul 20, 2022

dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Jul 20, 2022

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 20, 2022

dconeybe added 2 commits July 20, 2022 13:16

semaphore_test.cc: use firebase::Thread instead of the low-level pthr…

f3fd1f5

…eads library.

semaphore_test.cc: disable MultithreadedStressTest, since re-enabling…

54c9b26

… it is out-of-scope for this PR

dconeybe removed the tests: in-progress This PR's integration tests are in progress. label Jul 20, 2022

dconeybe assigned jonsimantov Jul 20, 2022

dconeybe requested a review from jonsimantov July 20, 2022 19:08

dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Jul 20, 2022

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 20, 2022

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 20, 2022

github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Jul 21, 2022

dconeybe removed the tests: failed This PR's integration tests failed. label Jul 21, 2022

Merge branch 'main' into dconeybe/SemaphoreTimedWaitUnitTest

798ee35

jonsimantov reviewed Jul 21, 2022

View reviewed changes

dconeybe removed the tests: succeeded This PR's integration tests succeeded. label Jul 24, 2022

dconeybe added 3 commits July 23, 2022 22:17

Merge branch 'main' into dconeybe/SemaphoreTimedWaitUnitTest

69bcc57

semaphore_test.cc: address code review feedback

ae0036d

semaphore_test.cc: keep track of the signal handler firing

9204a3a

dconeybe requested a review from jonsimantov July 25, 2022 14:54

dconeybe mentioned this pull request Jul 25, 2022

semaphore.h: handle spurious wakeups in TimedWait() on Linux #1021

Merged

dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Jul 25, 2022

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 25, 2022

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 25, 2022

github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Jul 25, 2022

jonsimantov approved these changes Jul 25, 2022

View reviewed changes

semaphore_test.cc: use the "k" prefix for constants

1ea2d0b

dconeybe removed tests: succeeded This PR's integration tests succeeded. tests: failed This PR's integration tests failed. labels Jul 26, 2022

dconeybe requested a review from jonsimantov July 26, 2022 04:13

dconeybe added the tests-requested: quick Trigger a quick set of integration tests. label Jul 26, 2022

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests-requested: quick Trigger a quick set of integration tests. labels Jul 26, 2022

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 26, 2022

dconeybe merged commit 69b870d into main Jul 26, 2022

dconeybe deleted the dconeybe/SemaphoreTimedWaitUnitTest branch July 26, 2022 17:57

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: failed This PR's integration tests failed. and removed tests: succeeded This PR's integration tests succeeded. labels Jul 26, 2022

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Jul 26, 2022

firebase locked and limited conversation to collaborators Aug 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

semaphore_test.cc: add TimedWaitSpuriousWakeupLinux #1037

semaphore_test.cc: add TimedWaitSpuriousWakeupLinux #1037

Uh oh!

dconeybe commented Jul 20, 2022

Uh oh!

github-actions bot commented Jul 20, 2022 •

edited

Loading

Uh oh!

jonsimantov Jul 21, 2022

Uh oh!

dconeybe Jul 25, 2022

Uh oh!

jonsimantov Jul 21, 2022

Uh oh!

dconeybe Jul 25, 2022

Uh oh!

jonsimantov Jul 21, 2022

Uh oh!

dconeybe Jul 25, 2022

Uh oh!

jonsimantov Jul 21, 2022

Uh oh!

dconeybe Jul 25, 2022

Uh oh!

jonsimantov Jul 21, 2022

Uh oh!

dconeybe Jul 25, 2022

Uh oh!

jonsimantov left a comment

Uh oh!

jonsimantov Jul 25, 2022

Uh oh!

dconeybe Jul 26, 2022

Uh oh!

dconeybe commented Jul 26, 2022

Uh oh!

Uh oh!

semaphore_test.cc: add TimedWaitSpuriousWakeupLinux #1037

semaphore_test.cc: add TimedWaitSpuriousWakeupLinux #1037

Uh oh!

Conversation

dconeybe commented Jul 20, 2022

Uh oh!

github-actions bot commented Jul 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

❌ Integration test FAILED

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonsimantov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dconeybe commented Jul 26, 2022

Uh oh!

Uh oh!

github-actions bot commented Jul 20, 2022 •

edited

Loading