Making DeviceCapabilities threadsafe with locking.

chromecast/base/device_capabilities.h

 // Copyright 2015 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.
 
 #ifndef CHROMECAST_BASE_DEVICE_CAPABILITIES_H_
 #define CHROMECAST_BASE_DEVICE_CAPABILITIES_H_
 
 #include <string>
 
 #include "base/memory/scoped_ptr.h"
@@ skipping 10 matching lines @@
 // value (base::Value). The class serves 2 main purposes:
 //
 // 1) Provide an interface for updating default capabilities and querying their
 // current value. Default capabilities are known to the system beforehand
 // and used by modules throughout Chromecast to control behavior of operations.
 //
 // 2) Store dynamic capabilities. Dynamic capabilities are not known to the
 // system beforehand and are introduced by external parties. These capabilites
 // are stored and then forwarded to app servers that use them to determine how
 // to interact with the device.
+//
+// Thread Safety:
+// Observers can be added from any thread. Each Observer is guaranteed to be
+// notified on same thread that it was added on and must be removed on the same
+// thread that it was added on.
+//
+// Validators can be registered from any thread. Each Validator's Validate()
+// method is guaranteed to be called on same thread that the Validator was
+// registered on. The Validator must be unregistered on the same thread
+// that it was registered on.
+//
+// All other methods can be called safely from any thread.
 
 // TODO(esum):
-// 1) A pure virtual interface with implementation files is no longer needed
-//    now that this file resides in chromecast/base. Change it such that there
-//    is just device_capabilities.h/cc.
-// 2) Make class thread-safe with locking.
-// 3) Add WifiSupported, HotspotSupported, and MultizoneSupported capabilities.
-// 4) It's not ideal to have the accessors (BluetoothSupported(), etc.) not
+// 1) Add WifiSupported, HotspotSupported, and MultizoneSupported capabilities.
+// 2) It's not ideal to have the accessors (BluetoothSupported(), etc.) not
 //    be valid initially until the capability gets registered. We might want
 //    to use some kind of builder class to solve this.
 class DeviceCapabilities {
  public:
   class Observer {
    public:
     // Called when DeviceCapabilities gets written to in any way. |path|
     // is full path to capability that has been updated.
     virtual void OnCapabilitiesChanged(const std::string& path) = 0;
 
@@ skipping 34 matching lines @@
    private:
     DeviceCapabilities* const capabilities_;
 
     DISALLOW_COPY_AND_ASSIGN(Validator);
   };
 
   // Default Capability keys
   static const char kKeyBluetoothSupported[];
   static const char kKeyDisplaySupported[];
 
+  // This class should get destroyed after all Validators have been
+  // unregistered, all Observers have been removed, and the class is no longer
+  // being accessed.
   virtual ~DeviceCapabilities() {}
 
   // Create empty instance with no capabilities. Although the class is not
   // singleton, there is meant to be a single instance owned by another module.
   // The instance should be created early enough for all managers to register
   // themselves, and then live long enough for all managers to unregister.
   static scoped_ptr<DeviceCapabilities> Create();
   // Creates an instance where all the default capabilities are initialized
   // to a predefined default value, and no Validators are registered. For use
   // only in unit tests.
   static scoped_ptr<DeviceCapabilities> CreateForTesting();
 
   // Registers a Validator for a capability. A given key must only be
   // registered once, and must be unregistered before calling Register() again.
   // If the capability has a value of Dictionary type, |key| must be just
   // the capability's top-level key and not include path expansions to levels
   // farther down. For example, "foo" is a valid value for |key|, but "foo.bar"
   // is not. Note that if "foo.bar" is updated in SetCapability(), the
   // Validate() method for "foo"'s Validator will be called, with a |path| of
-  // "foo.bar". Both Register() and Unregister() must be called on cast
-  // receiver main thread; the Validator provided will also be called on cast
-  // receiver main thread. Note that this method does not add or modify
-  // the capability. To do this, SetCapability() should be called, or
-  // Validators can call SetValidatedValue().
+  // "foo.bar". Note that this method does not add or modify the capability.
+  // To do this, SetCapability() should be called, or Validators can call
+  // SetValidatedValue(). This method is synchronous to ensure Validators know
+  // exactly when they may start receiving validation requests.
   virtual void Register(const std::string& key,
                         Validator* validator) = 0;
   // Unregisters Validator for |key|. |validator| argument must match
   // |validator| argument that was passed in to Register() for |key|. Note that
-  // the capability and its value remain untouched.
+  // the capability and its value remain untouched. This method is synchronous
+  // to ensure Validators know exactly when they will stop receiving validation
+  // requests.
   virtual void Unregister(const std::string& key,
                           const Validator* validator) = 0;
   // Gets the Validator currently registered for |key|. Returns nullptr if
   // no Validator is registered.
   virtual Validator* GetValidator(const std::string& key) const = 0;
 
   // Accessors for default capabilities. Note that the capability must be added
   // through SetCapability() or SetValidatedValue() (for Validators) before
   // accessors are called.
   virtual bool BluetoothSupported() const = 0;
   virtual bool DisplaySupported() const = 0;
 
-  // Gets the value of |path|. If capability at |path| does not exist,
-  // |out_value| remains untouched. Returns true if the capability has been
-  // successfully retrieved. Note that this does NOT perform a deep copy, and
-  // DeviceCapabilities still owns the memory returned through |out_value|.
-  virtual bool GetCapability(const std::string& path,
-                             const base::Value** out_value) const = 0;
+  // Returns a deep copy of the value at |path|. If the capability at |path|
+  // does not exist, a null scoped_ptr is returned.
+  virtual scoped_ptr<base::Value> GetCapability(
+      const std::string& path) const = 0;
 
   // Returns the complete capability string (JSON format).
-  virtual const std::string& GetCapabilitiesString() const = 0;
+  virtual std::string GetCapabilitiesString() const = 0;
 
-  // Returns the capabilities dictionary.
-  virtual const base::DictionaryValue* GetCapabilities() const = 0;
+  // Returns deep copy of the capabilities dictionary.
+  virtual scoped_ptr<base::DictionaryValue> GetCapabilities() const = 0;

[byungchul, 2015/10/28 01:09:43]

Can we avoid deep copy? Why not use ref-count here as well?

[esum, 2015/10/28 04:16:27]

Hmm. We can, but I can't think of a way to do that without somehow exposing the
internal wrapper class that makes base::DictionaryValue RefCountedThreadSafe. I
guess we could expose that wrapper publicly, but I personally don't think it's
very clean.

[byungchul, 2015/10/28 16:04:18]

Can you check how capabilities are used? If deep copy happens once per hours, I
am fine with deep copy with comments that why deep copy is not that a problem.
Otherwise, you may have to define a kind of CapabilitityGetter or Accessor which
holds scoped_refprt capabilites and defines these GetFunctions.

[esum, 2015/10/28 17:58:50]

GetCapabilities() gets called in EndPointWithApplication::SendReadyToReceiver(),
which I think gets called once right after application gets launched? So I think
it's not necessarily once per hours. But it's also not very frequent.

GetCapabilitiesString() gets called only when capabilities are written through
CastReceiverImpl::OnCapabilitiesChanged().

GetCapability() currently is not getting called.

What do you think?

[byungchul, 2015/10/28 18:37:16]

It would be nice to mention that these api are not expected to be called
frequently, otherwise need better approach.

[esum, 2015/10/28 18:44:30]

Hmm, I'm not happy leaving a comment like that. Let me think about a better
approach.

[esum, 2015/10/28 19:58:42]

I can think there are 2 straightforward ways of doing this:

1) Expose a public class nested in DeviceCapabilities like this:

class DictionaryReference {
 public:
  const DictionaryValue& Get() {
    return capabilities_data_->dictionary();
  }

 private:
  friend class DeviceCapabilitiesImpl;

  DictionaryReference(const scoped_refptr<ImmutableCapabilitiesData>&
capabilities_data) : capabilities_data_(capabilities_data) {}

  scoped_refptr<ImmutableCapabilitiesData> capabilities_data_;
}

The interface/implementation would look like:
DictionaryReference DeviceCapabilitiesImpl::GetCapabilities() {
  scoped_refptr<ImmutableCapabilitiesData> capabilities_data_ref =
      GetCapabilitiesDataReference();
  return DictionaryReference(capabilities_data_ref);
}

This way we are hiding the internal ImmutableCapabilitiesData wrapper class, and
there's just a simple public way of accessing returned the dictionary reference.
We would do same for GetCapabilitiesString().

2) In both uses of GetCapabilities() and GetCapabilitiesString(), the client
code is performing a deep copy anyways once it gets the return value. With this
interface as it is now, there are 2 deep copies (one to receive return value,
then once again in client code). We can bring this back down to 1 deep copy by
making the interface:

void DeviceCapabilitiesImpl::LoadCapabilities(scoped_ptr<DictionaryValue>*
destination) {
  DCHECK(destination);
  scoped_refptr<ImmutableCapabilitiesData> capabilities_data_ref =
      GetCapabilitiesDataReference();
  *destination = capabilities_data_ref->dictionary().CreateDeepCopy();
}

This way we are doing a deep copy but at least deep copying into the ultimate
destination. This would bring us back to the same performance as we had before
we made any of these changes.

Which if any of these do you prefer?

 
   // Updates the value at |path| to |proposed_value| if |path| already exists
   // and adds new capability if |path| doesn't. Note that if a key has been
   // registered that is at the beginning of |path|, then the Validator will be
   // used to determine if |proposed_value| is accepted.
   // Ex: If "foo" has a Validator registered, a |path| of "foo.bar"
   // will cause |proposed_value| to go through the Validator's Validate()
   // method. Client code may use the Observer interface to determine the
-  // ultimate value used.
+  // ultimate value used. This method is asynchronous.
   virtual void SetCapability(const std::string& path,
                              scoped_ptr<base::Value> proposed_value) = 0;
   // Iterates through entries in |dict_value| and calls SetCapability() for
-  // each one.
+  // each one. This method is asynchronous.
   virtual void MergeDictionary(const base::DictionaryValue& dict_value) = 0;
 
   // Adds/removes an observer. It doesn't take the ownership of |observer|.
   virtual void AddCapabilitiesObserver(Observer* observer) = 0;
   virtual void RemoveCapabilitiesObserver(Observer* observer) = 0;
 
  protected:
   DeviceCapabilities() {}
 
  private:
   // Does actual internal update of |path| to |new_value|.
-  virtual void SetValidatedValueInternal(const std::string& path,
-                                         scoped_ptr<base::Value> new_value) = 0;
+  virtual void SetValidatedValue(const std::string& path,
+                                 scoped_ptr<base::Value> new_value) = 0;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceCapabilities);
 };
 
 }  // namespace chromecast
 
 #endif  // CHROMECAST_BASE_DEVICE_CAPABILITIES_H_