Merge "Fix bug in the way an Event handles a bound thread." into oc-mr1-dev

am: 2b395216c3

Change-Id: I03d19bae08fe09da29d27e6297b443d07ee970e2

neuralnetworks/1.0/vts/functional/Event.cpp

@@ -10,9 +10,15 @@ namespace implementation {
 Event::Event() : mStatus(Status::WAITING) {}
 
 Event::~Event() {
-    if (mThread.joinable()) {
-        mThread.join();
-    }
+    // Note that we cannot call Event::join_thread from here: Event is
+    // intended to be reference counted, and it is possible that the
+    // reference count drops to zero in the bound thread, causing the
+    // bound thread to call this destructor. If a thread tries to join
+    // itself, it throws an exception, producing a message like the
+    // following:
+    //
+    //     terminating with uncaught exception of type std::__1::system_error:
+    //     thread::join failed: Resource deadlock would occur
 }
 
 Return<void> Event::notify(ReturnedStatus status) {
@@ -38,6 +44,7 @@ Event::Status Event::poll() {
 Event::Status Event::wait() {
     std::unique_lock<std::mutex> lock(mMutex);
     mCondition.wait(lock, [this]{return mStatus != Status::WAITING;});
+    join_thread_locked();
     return mStatus;
 }
 
@@ -69,6 +76,17 @@ bool Event::bind_thread(std::thread&& asyncThread) {
     return true;
 }
 
+void Event::join_thread() {
+    std::lock_guard<std::mutex> lock(mMutex);
+    join_thread_locked();
+}
+
+void Event::join_thread_locked() {
+    if (mThread.joinable()) {
+        mThread.join();
+    }
+}
+
 }  // namespace implementation
 }  // namespace V1_0
 }  // namespace neuralnetworks

neuralnetworks/1.0/vts/functional/Event.h

@@ -136,12 +136,21 @@ struct Event : public IEvent {
      * TODO: What if notify has already been called before on_finish?
      * TODO: Why does the return value of the callback matter?
      */
-     bool on_finish(std::function<bool(void)> callback);
+    bool on_finish(std::function<bool(void)> callback);
 
     /**
-     * Event::bind_thread binds a thread to the event ensuring that the thread
-     * has fully finished and cleaned its resources before the event is
-     * destroyed. The thread should be bound using std::move.
+     * Event::bind_thread binds a thread to the event for later use by
+     * Event::join_thread.
+     *
+     * The thread must be passed using std::move.
+     *
+     * Once a thread is bound with Event::bind_thread, the client code
+     * should ensure that one of the following occurs before the event is
+     * destroyed:
+     * - Event::join_thread has been called.
+     * - Event::wait has been called.
+     * - Event::wait_for has been called and returned other than TIMEOUT.
+     * - Event::wait_until has been called and returned other than TIMEOUT.
      *
      * The bound thread shall not call any Event method with the exception of
      * IEvent::notify, which it will call when the thread has finished its
@@ -154,9 +163,20 @@ struct Event : public IEvent {
      *                    asyncThread.joinable() must be true.
      * @return bool True if successful, false if thread was not properly bound.
      */
-     bool bind_thread(std::thread&& asyncThread);
+    bool bind_thread(std::thread&& asyncThread);
+
+    /**
+     * Event::join_thread ensures that the thread (if any) bound to
+     * this event with Event::bind_thread has fully finished and
+     * cleaned its resources. It is legal to call this function
+     * multiple times, concurrently or sequentially.
+     */
+    void join_thread();
 
  private:
+    // Same as Event::join_thread but assumes we already hold a lock on mMutex.
+    void join_thread_locked();
+
     Status                    mStatus;
     std::mutex                mMutex;
     std::condition_variable   mCondition;
@@ -172,6 +192,9 @@ Event::Status Event::wait_for(const std::chrono::duration<Rep,Period>& timeout_d
     std::unique_lock<std::mutex> lock(mMutex);
     std::cv_status status = mCondition.wait_for(lock, timeout_duration,
                                                 [this]{return mStatus != Status::WAITING;});
+    if (status != std::cv_status::timeout) {
+        join_thread_locked();
+    }
     return status != std::cv_status::timeout ? mStatus : Status::TIMEOUT;
 }
 
@@ -180,6 +203,9 @@ Event::Status Event::wait_until(const std::chrono::time_point<Clock,Duration>& t
     std::unique_lock<std::mutex> lock(mMutex);
     std::cv_status status = mCondition.wait_until(lock, timeout_time,
                                                   [this]{return mStatus != Status::WAITING;});
+    if (status != std::cv_status::timeout) {
+        join_thread_locked();
+    }
     return status != std::cv_status::timeout ? mStatus : Status::TIMEOUT;
 }