Properly reset NONBLOCK flag in semaphore_try_wait()

Without this fix, calling semaphore_try_wait() on a semaphore that
wasn't currently set, would leave the NONBLOCK flag on the file
descriptor as a side-effect.

Also added a unit test for semaphores, including a test specifically for
this condition.

Change-Id: I0ea37bb68b14c76febaab25b3aee1bb4f5acee8c

diff --git a/osi/Android.mk b/osi/Android.mk
index c417505..40cedc2 100644 (file)
--- a/osi/Android.mk
+++ b/osi/Android.mk
@@ -67,6 +67,7 @@ btosiCommonTestSrc := \
     ./test/list_test.cpp \
     ./test/reactor_test.cpp \
     ./test/ringbuffer_test.cpp \
+    ./test/semaphore_test.cpp \
     ./test/thread_test.cpp \
     ./test/time_test.cpp
 

diff --git a/osi/src/semaphore.c b/osi/src/semaphore.c
index 8693c9c..59b250a 100644 (file)
--- a/osi/src/semaphore.c
+++ b/osi/src/semaphore.c
@@ -85,13 +85,14 @@ bool semaphore_try_wait(semaphore_t *semaphore) {
     return false;
   }
 
+  bool rc = true;
   eventfd_t value;
   if (eventfd_read(semaphore->fd, &value) == -1)
-    return false;
+    rc = false;
 
   if (fcntl(semaphore->fd, F_SETFL, flags) == -1)
-    LOG_ERROR(LOG_TAG, "%s unable to resetore flags for semaphore fd: %s", __func__, strerror(errno));
-  return true;
+    LOG_ERROR(LOG_TAG, "%s unable to restore flags for semaphore fd: %s", __func__, strerror(errno));
+  return rc;
 }
 
 void semaphore_post(semaphore_t *semaphore) {

diff --git a/osi/test/semaphore_test.cpp b/osi/test/semaphore_test.cpp
new file mode 100644 (file)
index 0000000..c30dd0b
--- /dev/null
+++ b/osi/test/semaphore_test.cpp
@@ -0,0 +1,87 @@
+#include <gtest/gtest.h>
+
+#include "AllocationTestHarness.h"
+
+extern "C" {
+#include <unistd.h>
+#include <sys/select.h>
+
+#include "osi/include/osi.h"
+#include "osi/include/reactor.h"
+#include "osi/include/semaphore.h"
+#include "osi/include/thread.h"
+}
+
+struct SemaphoreTestSequenceHelper {
+  semaphore_t *semaphore;
+  int counter;
+};
+
+namespace {
+  void sleep_then_increment_counter(void *context) {
+    SemaphoreTestSequenceHelper *helper =
+            reinterpret_cast<SemaphoreTestSequenceHelper*>(context);
+    assert(helper);
+    assert(helper->semaphore);
+    sleep(1);
+    ++helper->counter;
+    semaphore_post(helper->semaphore);
+  }
+} // namespace
+
+class SemaphoreTest : public AllocationTestHarness {};
+
+TEST_F(SemaphoreTest, test_new_simple) {
+  semaphore_t *semaphore = semaphore_new(0);
+  ASSERT_TRUE(semaphore != NULL);
+  semaphore_free(semaphore);
+}
+
+TEST_F(SemaphoreTest, test_new_with_value) {
+  semaphore_t *semaphore = semaphore_new(3);
+  ASSERT_TRUE(semaphore != NULL);
+
+  EXPECT_TRUE(semaphore_try_wait(semaphore));
+  EXPECT_TRUE(semaphore_try_wait(semaphore));
+  EXPECT_TRUE(semaphore_try_wait(semaphore));
+  EXPECT_FALSE(semaphore_try_wait(semaphore));
+
+  semaphore_free(semaphore);
+}
+
+TEST_F(SemaphoreTest, test_try_wait) {
+  semaphore_t *semaphore = semaphore_new(0);
+  ASSERT_TRUE(semaphore != NULL);
+
+  EXPECT_FALSE(semaphore_try_wait(semaphore));
+  semaphore_post(semaphore);
+  EXPECT_TRUE(semaphore_try_wait(semaphore));
+  EXPECT_FALSE(semaphore_try_wait(semaphore));
+
+  semaphore_free(semaphore);
+}
+
+TEST_F(SemaphoreTest, test_wait_after_post) {
+  semaphore_t *semaphore = semaphore_new(0);
+  ASSERT_TRUE(semaphore != NULL);
+  semaphore_post(semaphore);
+  semaphore_wait(semaphore);
+  semaphore_free(semaphore);
+}
+
+TEST_F(SemaphoreTest, test_ensure_wait) {
+  semaphore_t *semaphore = semaphore_new(0);
+  ASSERT_TRUE(semaphore != NULL);
+  thread_t *thread = thread_new("semaphore_test_thread");
+  ASSERT_TRUE(thread != NULL);
+
+  EXPECT_FALSE(semaphore_try_wait(semaphore));
+  SemaphoreTestSequenceHelper sequence_helper = {semaphore, 0};
+  thread_post(thread, sleep_then_increment_counter, &sequence_helper);
+  semaphore_wait(semaphore);
+  EXPECT_EQ(sequence_helper.counter, 1) <<
+      "semaphore_wait() did not wait for counter to increment";
+
+  semaphore_free(semaphore);
+  thread_free(thread);
+}

diff --git a/osi/test/thread_test.cpp b/osi/test/thread_test.cpp
index d624d7d..b45365d 100644 (file)
--- a/osi/test/thread_test.cpp
+++ b/osi/test/thread_test.cpp
@@ -6,7 +6,6 @@ extern "C" {
 #include <sys/select.h>
 
 #include "osi/include/reactor.h"
-#include "osi/include/semaphore.h"
 #include "osi/include/thread.h"
 #include "osi/include/osi.h"
 }