DtlsIdentityStore[Interface/Impl] updated, DtlsIdentityService to be removed

talk/app/webrtc/dtlsidentitystore.cc

Index: talk/app/webrtc/dtlsidentitystore.cc
diff --git a/talk/app/webrtc/dtlsidentitystore.cc b/talk/app/webrtc/dtlsidentitystore.cc
index abd35e1fab522c47334f432bc2ccb7bba6888e17..fa11daba81687fa3951e9612a119796d6a93c8ee 100644
--- a/talk/app/webrtc/dtlsidentitystore.cc
+++ b/talk/app/webrtc/dtlsidentitystore.cc
@@ -30,32 +30,33 @@
 #include "talk/app/webrtc/webrtcsessiondescriptionfactory.h"
 #include "webrtc/base/logging.h"
 
-using webrtc::DTLSIdentityRequestObserver;
+using webrtc::DtlsIdentityRequestObserver;
 using webrtc::WebRtcSessionDescriptionFactory;
 
 namespace webrtc {
 
+const char* DtlsIdentityStoreImpl::common_name_ = "WebRTC";
+
 namespace {
 
 enum {
   MSG_DESTROY,
   MSG_GENERATE_IDENTITY,
-  MSG_GENERATE_IDENTITY_RESULT,
-  MSG_RETURN_FREE_IDENTITY
+  MSG_GENERATE_IDENTITY_RESULT
 };
 
-typedef rtc::ScopedMessageData<rtc::SSLIdentity> IdentityResultMessageData;
-
 }  // namespace
 
 // This class runs on the worker thread to generate the identity. It's necessary
 // to separate this class from DtlsIdentityStore so that it can live on the
 // worker thread after DtlsIdentityStore is destroyed.
-class DtlsIdentityStore::WorkerTask : public sigslot::has_slots<>,
-                                      public rtc::MessageHandler {
+class DtlsIdentityStoreImpl::WorkerTask : public sigslot::has_slots<>,
+                                          public rtc::MessageHandler {
  public:
-  explicit WorkerTask(DtlsIdentityStore* store)
-      : signaling_thread_(rtc::Thread::Current()), store_(store) {
+  WorkerTask(DtlsIdentityStoreImpl* store, rtc::KeyType key_type)
+      : signaling_thread_(rtc::Thread::Current()),
+        store_(store),
+        key_type_(key_type) {
     store_->SignalDestroyed.connect(this, &WorkerTask::OnStoreDestroyed);
   }
 
@@ -63,13 +64,18 @@ class DtlsIdentityStore::WorkerTask : public sigslot::has_slots<>,
 
  private:
   void GenerateIdentity_w() {
+    // TODO(hbos): Use key_type_ when torbjorng's CL has landed.
+    LOG(LS_INFO) << "Generating identity. Key type (TODO(hbos): should use): "
+                 << key_type_;
     rtc::scoped_ptr<rtc::SSLIdentity> identity(
-        rtc::SSLIdentity::Generate(DtlsIdentityStore::kIdentityName));
+        rtc::SSLIdentity::Generate(common_name_));
 
     {
       rtc::CritScope cs(&cs_);
       if (store_) {
-        store_->PostGenerateIdentityResult_w(identity.Pass());
+        IdentityResultMessageData* msg = new IdentityResultMessageData(
+            new IdentityResult(key_type_, identity.release()));

[tommi, 2015/06/18 13:55:35]

would be better to do identity.Pass() here I think (i.e. change the ctor)

[hbos, 2015/06/18 15:45:48]

Acknowledged.

+        signaling_thread_->Post(store_, MSG_GENERATE_IDENTITY_RESULT, msg);

[tommi, 2015/06/18 13:55:35]

I think we can make the interface between the WorkerTask and store_ a little
cleaner and also avoid requiring a lock for store_.

* Here, don't Post() to store_.  Instead post to the instance itself
(WorkerTask).
* Make sure (DCHECK) that store_ is only touched on the signaling thread.
* In OnStoreDestroyed(), DCHECK that you're on the signaling thread and then
just set store_ to nullptr.
* in WorkerTask::OnMessage, handle MSG_GENERATE_IDENTITY_RESULT, DCHECK you're
on the signaling thread and check if store_ is nullptr.  If it's not null, call
OnIdentityGenerated() on the store_ with the results and delete the message.

[hbos, 2015/06/18 15:45:48]

<This has not been addressed yet>

[hbos, 2015/07/02 09:32:23]

Done!

       }
     }
   }
@@ -100,136 +106,139 @@ class DtlsIdentityStore::WorkerTask : public sigslot::has_slots<>,
   }
 
   rtc::Thread* const signaling_thread_;
+  // Locking this prevents |store_| from being destroyed if it has not already
+  // been so.

[tommi, 2015/06/18 13:55:35]

don't think we need this lock

[hbos, 2015/06/18 15:45:48]

<This has not been addressed yet>

[hbos, 2015/07/02 09:32:23]

Acknowledged.

   rtc::CriticalSection cs_;
-  DtlsIdentityStore* store_;
+  DtlsIdentityStoreImpl* store_;
+  rtc::KeyType key_type_;

[tommi, 2015/06/18 13:55:35]

const?

[hbos, 2015/06/18 15:45:48]

Acknowledged.

 };
 
-// Arbitrary constant used as common name for the identity.
-// Chosen to make the certificates more readable.
-const char DtlsIdentityStore::kIdentityName[] = "WebRTC";
-
-DtlsIdentityStore::DtlsIdentityStore(rtc::Thread* signaling_thread,
-                                     rtc::Thread* worker_thread)
+DtlsIdentityStoreImpl::DtlsIdentityStoreImpl(rtc::Thread* signaling_thread,
+                                             rtc::Thread* worker_thread)
     : signaling_thread_(signaling_thread),
       worker_thread_(worker_thread),
-      pending_jobs_(0) {}
+      request_observers_(),
+      gen_in_progress_counts_(),
+      free_identities_() { }
 
-DtlsIdentityStore::~DtlsIdentityStore() {
+DtlsIdentityStoreImpl::~DtlsIdentityStoreImpl() {
   SignalDestroyed();

[tommi, 2015/06/18 13:55:35]

We should always be on the signaling thread here, right?

[hbos, 2015/06/18 15:45:47]

I think so, want a DCHECK thread check?
<This has not been addressed yet>

[hbos, 2015/07/02 09:32:23]

Added DCHECK.

 }
 
-void DtlsIdentityStore::Initialize() {
+void DtlsIdentityStoreImpl::Initialize() {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
-  // Do not aggressively generate the free identity if the worker thread and the
-  // signaling thread are the same.
+  // Preemptively generate identities unless the worker thread and signaling
+  // thread are the same (only do preemptive work in the background).
   if (worker_thread_ != signaling_thread_) {
-    GenerateIdentity();
+    // Only necessary for RSA.
+    GenerateIdentity(rtc::KT_RSA, nullptr);
   }
 }
 
-void DtlsIdentityStore::RequestIdentity(DTLSIdentityRequestObserver* observer) {
+void DtlsIdentityStoreImpl::RequestIdentity(
+    rtc::KeyType key_type, ScopedRefPtrObserver observer) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
   DCHECK(observer);
 
-  // Must return the free identity async.
-  if (free_identity_.get()) {
-    IdentityResultMessageData* msg =
-        new IdentityResultMessageData(free_identity_.release());
-    signaling_thread_->Post(this, MSG_RETURN_FREE_IDENTITY, msg);
-  }
-
-  pending_observers_.push(observer);
-  GenerateIdentity();
+  GenerateIdentity(key_type, observer);
 }
 
-void DtlsIdentityStore::OnMessage(rtc::Message* msg) {
+void DtlsIdentityStoreImpl::OnMessage(rtc::Message* msg) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
   switch (msg->message_id) {
     case MSG_GENERATE_IDENTITY_RESULT: {
       rtc::scoped_ptr<IdentityResultMessageData> pdata(
           static_cast<IdentityResultMessageData*>(msg->pdata));
-      OnIdentityGenerated(pdata->data().Pass());
-      break;
-    }
-    case MSG_RETURN_FREE_IDENTITY: {
-      rtc::scoped_ptr<IdentityResultMessageData> pdata(
-          static_cast<IdentityResultMessageData*>(msg->pdata));
-      ReturnIdentity(pdata->data().Pass());
+      OnIdentityGenerated(pdata->data()->key_type_,
+                          pdata->data()->identity_.Pass());
       break;
     }
   }
 }
 
-bool DtlsIdentityStore::HasFreeIdentityForTesting() const {
+bool DtlsIdentityStoreImpl::HasFreeIdentityForTesting(
+    rtc::KeyType key_type) const {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
-  return free_identity_.get() != nullptr;
+  return free_identities_[key_type].get() != nullptr;
 }
 
-void DtlsIdentityStore::GenerateIdentity() {
+void DtlsIdentityStoreImpl::GenerateIdentity(
+    rtc::KeyType key_type, ScopedRefPtrObserver observer) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
-  pending_jobs_++;
-  LOG(LS_VERBOSE) << "New DTLS identity generation is posted, "
-                  << "pending_identities=" << pending_jobs_;
 
-  WorkerTask* task = new WorkerTask(this);
+  // Enqueue observer to be informed when generation of |key_type| is completed.
+  if (observer.get()) {
+    request_observers_[key_type].push_back(observer);
+
+    // Already have a free identity generated?
+    if (free_identities_[key_type].get()) {
+      // Return identity async - post even though we are on |signaling_thread_|.
+      LOG(LS_VERBOSE) << "Using a free DTLS identity.";
+      IdentityResultMessageData* msg = new IdentityResultMessageData(
+          new IdentityResult(key_type, free_identities_[key_type].release()));
+      ++gen_in_progress_counts_[key_type];
+      signaling_thread_->Post(this, MSG_GENERATE_IDENTITY_RESULT, msg);
+      return;
+    }
+
+    // Free identity in the process of being generated?
+    if (gen_in_progress_counts_[key_type] ==
+          request_observers_[key_type].size()) {
+      // No need to do anything, the free identity will be returned to the
+      // observer in a MSG_GENERATE_IDENTITY_RESULT.
+      return;
+    }
+  }
+
+  // Enqueue/Post a worker task to do the generation.
+  WorkerTask* task = new WorkerTask(this, key_type);  // Post 1 task/request.
   // The WorkerTask is owned by the message data to make sure it will not be
   // leaked even if the task does not get run.
-  IdentityTaskMessageData* msg = new IdentityTaskMessageData(task);
+  WorkerTaskMessageData* msg = new WorkerTaskMessageData(task);
+  ++gen_in_progress_counts_[key_type];
   worker_thread_->Post(task, MSG_GENERATE_IDENTITY, msg);
 }
 
-void DtlsIdentityStore::OnIdentityGenerated(
-    rtc::scoped_ptr<rtc::SSLIdentity> identity) {
+void DtlsIdentityStoreImpl::OnIdentityGenerated(
+    rtc::KeyType key_type, rtc::scoped_ptr<rtc::SSLIdentity> identity) {
   DCHECK(rtc::Thread::Current() == signaling_thread_);
 
-  pending_jobs_--;
-  LOG(LS_VERBOSE) << "A DTLS identity generation job returned, "
-                  << "pending_identities=" << pending_jobs_;
+  DCHECK(gen_in_progress_counts_[key_type]);
+  --gen_in_progress_counts_[key_type];
 
-  if (pending_observers_.empty()) {
-    if (!free_identity_.get()) {
-      free_identity_.reset(identity.release());
-      LOG(LS_VERBOSE) << "A free DTLS identity is saved";
-    }
-    return;
+  ScopedRefPtrObserver observer = nullptr;
+  if (!request_observers_[key_type].empty()) {
+    observer = request_observers_[key_type].front();
+    request_observers_[key_type].pop_front();
   }
-  ReturnIdentity(identity.Pass());
-}
-
-void DtlsIdentityStore::ReturnIdentity(
-    rtc::scoped_ptr<rtc::SSLIdentity> identity) {
-  DCHECK(rtc::Thread::Current() == signaling_thread_);
-  DCHECK(!free_identity_.get());
-  DCHECK(!pending_observers_.empty());
 
-  rtc::scoped_refptr<DTLSIdentityRequestObserver> observer =
-      pending_observers_.front();
-  pending_observers_.pop();
-
-  if (identity.get()) {
-    observer->OnSuccessWithIdentityObj(identity.Pass());
+  if (observer.get() == nullptr) {
+    // No observer - store result in |free_identities_|.
+    DCHECK(!free_identities_[key_type].get());
+    free_identities_[key_type].reset(identity.release());

[tommi, 2015/06/18 13:55:35]

nit: swap()?

[hbos, 2015/06/18 15:45:47]

Acknowledged.

+    if (free_identities_[key_type].get())
+      LOG(LS_VERBOSE) << "A free DTLS identity was saved.";
+    else
+      LOG(LS_WARNING) << "Failed to generate DTLS identity (preemptively).";
   } else {
-    // Pass an arbitrary error code.
-    observer->OnFailure(0);
-    LOG(LS_WARNING) << "Failed to generate SSL identity";
-  }
+    // Return the result to the observer.
+    if (identity.get()) {
+      LOG(LS_VERBOSE) << "A DTLS identity is returned to an observer.";
+      observer->OnSuccessWithIdentityObj(identity.Pass());
+    } else {
+      LOG(LS_WARNING) << "Failed to generate DTLS identity.";
+      observer->OnFailure(0);
+    }
 
-  // Do not aggressively generate the free identity if the worker thread and the
-  // signaling thread are the same.
-  if (worker_thread_ != signaling_thread_ &&
-      pending_observers_.empty() &&
-      pending_jobs_ == 0) {
-    // Generate a free identity in the background.
-    GenerateIdentity();
+    // Preemptively generate another identity of the same type?
+    if (worker_thread_ != signaling_thread_  // Only do on a background thread.
+        && (key_type == rtc::KT_RSA)         // Only necessary for RSA.

[tommi, 2015/06/18 13:55:35]

&& at the end of the previous line

[hbos, 2015/06/18 15:45:48]

Acknowledged.

+        && !free_identities_[key_type].get()
+        && request_observers_[key_type].size() <=
+           gen_in_progress_counts_[key_type]) {
+      GenerateIdentity(key_type, nullptr);
+    }
   }
 }
 
-void DtlsIdentityStore::PostGenerateIdentityResult_w(
-    rtc::scoped_ptr<rtc::SSLIdentity> identity) {
-  DCHECK(rtc::Thread::Current() == worker_thread_);
-
-  IdentityResultMessageData* msg =
-      new IdentityResultMessageData(identity.release());
-  signaling_thread_->Post(this, MSG_GENERATE_IDENTITY_RESULT, msg);
-}
 }  // namespace webrtc