[NaCl SDK} Add EventListener and EventEmitter to support epoll.

native_client_sdk/src/libraries/nacl_io/event_listener.cc

Index: native_client_sdk/src/libraries/nacl_io/event_listener.cc
diff --git a/native_client_sdk/src/libraries/nacl_io/event_listener.cc b/native_client_sdk/src/libraries/nacl_io/event_listener.cc
new file mode 100644
index 0000000000000000000000000000000000000000..b7b78cdbc9a18d076f2f08ac3f56bac7cd6c9282
--- /dev/null
+++ b/native_client_sdk/src/libraries/nacl_io/event_listener.cc
@@ -0,0 +1,243 @@
+/* Copyright (c) 2013 The Chromium Authors. All rights reserved.
+ * Use of this source code is governed by a BSD-style license that can be
+ * found in the LICENSE file.

[Sam Clegg, 2013/07/17 17:34:56]

C++ comment

+ */

[Sam Clegg, 2013/07/17 17:34:56]

C++ comment

+
+#include <errno.h>
+#include <pthread.h>
+#include <stdio.h>
+#include <sys/stat.h>
+#include <sys/time.h>
+#include <time.h>
+#include <unistd.h>
+
+#include "nacl_io/event_listener.h"

[Sam Clegg, 2013/07/17 17:34:56]

Put this header first.

[Sam Clegg, 2013/07/17 17:34:56]

put this header first?

+#include "sdk_util/auto_lock.h"
+
+
+EventListener::EventListener() {
+  pthread_cond_init(&signal_cond_, NULL);
+}
+
+EventListener::~EventListener() {
+  pthread_cond_destroy(&signal_cond_);
+}
+
+// Before we can destroy ourselves, we must first unregister all the
+// EventInfo objects from the various EventListeners
+void EventListener::Destroy() {
+  EventInfoMap_t::iterator it;
+
+  // We must always hold the EventListener lock prior to calling then

[binji, 2013/07/17 22:23:20]

s/then/the/

[noelallen1, 2013/07/18 22:18:16]

Done.

+  // EventEmitter.
+  AutoLock lock(info_lock_);

[binji, 2013/07/17 22:23:20]

Is this necessary? If we are destroying doesn't that mean that this thread holds
the only reference to EventListener? Or can external users call the public
methods of EventListener without ref-counting it?

[noelallen1, 2013/07/18 22:18:16]

True, there should be no references to the listener at this point.
Done.

+  for (it = event_info_.begin(); it != event_info_.end(); it++) {
+    if (it->second->emitter) {
+      it->second->emitter->UnregisterEventInfo(it->second);
+    }
+  }
+
+  EventEmitter::Destroy();
+}
+
+uint32_t EventListener::GetEventStatus() {
+  // Always writable, but we can only assume it to be readable if there
+  // is an event waiting.

[Sam Clegg, 2013/07/17 17:34:56]

Why always writable?

[noelallen1, 2013/07/17 21:00:18]

You can always modify the EventListener, and write(efd...) will never block.

+  return signaled_.empty() ? KE_WRITE_READY : KE_WRITE_READY | KE_READ_READY;
+}
+
+int EventListener::GetType() {
+  // For lack of a better type, report socket to signify it can be in an
+  // used to signal.
+  return S_IFSOCK;
+}
+
+// Called by EventEmitter, wakes any blocking threads to verify if the wait
+// conditions have been met.
+void EventListener::Signal(const ScopedEventInfo& info) {
+  AutoLock lock(signal_lock_);
+  if (waiting_) {
+    signaled_.insert(info);
+    pthread_cond_signal(&signal_cond_);

[binji, 2013/07/17 22:23:20]

pthread_cond_broadcast?

[noelallen1, 2013/07/18 22:18:16]

Good question...  Do we have multiple threads waiting on the same Listener get
fired off in order, or do they all go...  Probably broadcast work would better
here than chain signalling.

The correct thing here is to broadcast, then one thread at a time will empty the
event set until the last one throws the remainders away.

+  }
+}
+
+Error EventListener::Wait(EventData* events,

[binji, 2013/07/17 22:23:20]

This interface could be simplified by passing a std::vector<EventData>* here.
Then max is irrelevant, and event_cnt is the resulting length.

[noelallen1, 2013/07/18 22:18:16]

This matches the final 'C' interface.  A vector would cause an extra copy, as
well as causing us to "loose" events since they would get dropped later.

+                          int max,
+                          int ms_timeout,
+                          int* event_cnt) {

[binji, 2013/07/17 22:23:20]

I've been using out_... for out parameters. Also, this name should match the
name in the header.

[noelallen1, 2013/07/18 22:18:16]

Done.

+  *event_cnt = 0;
+
+  if (max <= 0)
+    return EINVAL;
+
+  if (NULL == events)
+    return EFAULT;
+
+  {
+    AutoLock info_lock(info_lock_);
+
+    // Go through the "live" event infos and see if they are in a signaled state
+    EventInfoMap_t::iterator it = event_info_.begin();
+    while ((it != event_info_.end()) && (*event_cnt < max)) {
+      ScopedEventInfo& info = it->second;
+      uint32_t event_bits = info->emitter->GetEventStatus() & info->filter;
+
+      if (event_bits) {
+        events[*event_cnt].events = event_bits;
+        events[*event_cnt].data = info->data;
+        (*event_cnt)++;
+      }
+
+      it++;
+    }
+
+    // If we have anything or we aren't waiting then we can return early.
+    if ((*event_cnt >= max) || (ms_timeout == 0))

[binji, 2013/07/17 22:23:20]

The code and comment do not align: Is this supposed to return if anything is
available, or if events is completely full?

[noelallen1, 2013/07/18 22:18:16]

Done.

+      return 0;
+
+  } // End of info_lock scope.
+
+  // Compute the absolute time we can wait until.
+  struct timespec timeout;
+  struct timeval current_time;
+  if (ms_timeout >= 0) {
+    gettimeofday(&current_time, NULL);
+    timeout.tv_sec = current_time.tv_sec;
+    timeout.tv_nsec = current_time.tv_usec * 1000;
+    timeout.tv_nsec += ms_timeout * 1000000;
+    while (timeout.tv_nsec > 1000000000) {
+      timeout.tv_nsec -= 1000000000;
+      timeout.tv_sec += 1;
+    }
+  }

[binji, 2013/07/17 22:23:20]

helper function for this?

[noelallen1, 2013/07/18 22:18:16]

Done.

+
+  // Keep waiting while we have time left, or no events captured

[binji, 2013/07/17 22:23:20]

This comment does not align with the code -- ms_timeout never changes, so it's
not really "time left".

[noelallen1, 2013/07/18 22:18:16]

Done.

+  while ((0 == *event_cnt) && ms_timeout) {

[Sam Clegg, 2013/07/17 17:34:56]

be consistent about comparing ms_timeout explicitly with 0 (or not).

Is our convention to explicitly compare with 0?

Also be consistent about putting 0/NULL before or after ==.

[noelallen1, 2013/07/18 22:18:16]

Done.

[noelallen1, 2013/07/18 22:18:16]

Yes, (foo) (!foo) should be bool only.  Otherwise (0 != foo) (NULL == foo) to
prevent accidental assignment.

+    // We are now officially waiting.
+    AutoLock signal_lock(signal_lock_);
+    waiting_++;
+
+    // If we don't have any signals yet, wait for any Emitter to Signal or for
+    // the timeout to expire.  We temporarily unlock the data mutex to allow
+    while (signaled_.empty()) {
+      int retCode;

[Sam Clegg, 2013/07/17 17:34:56]

return_code?

[noelallen1, 2013/07/17 21:00:18]

Done.

+
+      if (ms_timeout >= 0) {
+        retCode = pthread_cond_timedwait(&signal_cond_,
+                                         signal_lock_.mutex(),
+                                         &timeout);
+      } else {
+        retCode = pthread_cond_wait(&signal_cond_, signal_lock_.mutex());
+      }
+
+      // If there is no error, then we may have been signaled.
+      if (retCode == 0)
+        break;
+
+      // For any error case:
+      if (retCode == ETIMEDOUT) {
+        // A "TIMEOUT" is not an error.
+        retCode = 0;
+      } else {
+        // Otherwise this has gone bad, so return EBADF.
+        retCode = EBADF;
+      }
+
+      waiting_--;
+      return retCode;
+    }
+
+    // Copy signals over as long as we have room
+    while (!signaled_.empty() && (*event_cnt < max)) {
+      EventInfoSet_t::iterator it = signaled_.begin();
+
+      events[*event_cnt].events = (*it)->events;
+      events[*event_cnt].data = (*it)->data;
+      (*event_cnt)++;
+
+      signaled_.erase(it);
+    }
+
+    // If we are the last thread waiting, clear out the signalled set
+    if (waiting_ == 1)
+      signaled_.clear();
+
+    // We are done waiting.
+    waiting_--;
+  }
+
+  return 0;
+}
+
+Error EventListener::Track(int id,
+                          const ScopedRef<EventEmitter>& emitter,
+                          uint32_t filter,
+                          uint64_t data) {
+  AutoLock lock(info_lock_);
+  EventInfoMap_t::iterator it = event_info_.find(id);
+
+  // If it's not a streaming type, then it can not be added.
+  if ((emitter->GetType() & (S_IFIFO | S_IFSOCK)) == 0)
+    return EPERM;

[Sam Clegg, 2013/07/17 17:34:56]

Can't you add regular files to an epoll set?

[noelallen1, 2013/07/17 21:00:18]

man epoll_ctl: 
  EPERM  The target file fd does not support epoll.

Verified by testing on Linux as well.

+
+  if (it != event_info_.end())
+    return EEXIST;
+
+  if (emitter.get() == this)
+    return EINVAL;
+
+  ScopedEventInfo info(new EventInfo);
+  info->emitter = emitter.get();
+  info->listener = this;
+  info->id = id;
+  info->filter = filter;
+  info->data = data;
+  info->events = 0;
+
+  emitter->RegisterEventInfo(info);
+  event_info_[id] = info;
+  return 0;
+}
+
+Error EventListener::Update(int id, uint32_t filter, uint64_t data) {
+  AutoLock lock(info_lock_);
+  EventInfoMap_t::iterator it = event_info_.find(id);
+  if (it == event_info_.end())
+    return ENOENT;
+
+  ScopedEventInfo& info = it->second;
+  info->filter = filter;
+  info->data = data;
+  return 0;
+}
+
+Error EventListener::Free(int id) {
+  AutoLock lock(info_lock_);
+  EventInfoMap_t::iterator it = event_info_.find(id);
+  if (it == event_info_.end())
+    return ENOENT;
+
+  if (it->second->emitter)

[binji, 2013/07/17 22:23:20]

is this check necessary? It looks like event_info_ should never have an
EventInfo with a NULL emitter (The EventInfo is removed when it is abandoned.)

[noelallen1, 2013/07/18 22:18:16]

Done.

[binji, 2013/07/19 20:47:18]

Still need the UnregisterEventInfo?

+    it->second->emitter->UnregisterEventInfo(it->second);
+
+  event_info_.erase(it);
+  return 0;
+}
+
+// Notify the EventEmitter is abandon and event.
+void EventListener::AbandonedEventInfo(const ScopedEventInfo& event) {
+  {
+    AutoLock lock(info_lock_);
+
+    // Remove events for emitters that are abandoned.
+    event->emitter = NULL;

[binji, 2013/07/17 22:23:20]

why? Who else could be using this EventInfo?

[noelallen1, 2013/07/18 22:18:16]

The EventInfo can be in:
   event_info_map_ only - no signal while someone is waiting.
   event_info_map_ and signaled_ - signal while someone is waiting.
   signaled_ only - abandoned, but the "exception/hang up" has yet to be
consumed.

This is why the EventInfo is ref counted.

+    event_info_.erase(event->id);
+  }
+
+  // Force to KE_SHUTDOWN since this will no longer be read/writable.
+  event->events = KE_SHUTDOWN;
+  Signal(event);
+}
+
+

[Sam Clegg, 2013/07/17 17:34:56]

Remove trailing newlines.

[noelallen1, 2013/07/17 21:00:18]

Done.