tracing: the agent registry implementation

services/resource_coordinator/tracing/agent_registry.cc

+// Copyright 2017 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.
+
+#include "services/resource_coordinator/tracing/agent_registry.h"
+
+#include "base/callback_forward.h"
+#include "base/threading/thread_checker.h"
+#include "services/service_manager/public/cpp/bind_source_info.h"
+
+namespace {
+tracing::AgentRegistry* g_agent_registry;
+}
+
+namespace tracing {
+
+AgentRegistry::Entry::Entry(size_t id,
+                            AgentRegistry* agent_registry,
+                            mojom::AgentPtr agent,
+                            const std::string& label,
+                            mojom::TraceDataType type,
+                            bool supports_explicit_clock_sync)
+    : id_(id),
+      agent_registry_(agent_registry),
+      agent_(std::move(agent)),
+      label_(label),
+      type_(type),
+      supports_explicit_clock_sync_(supports_explicit_clock_sync) {
+  DCHECK(label.size());

[Primiano Tucci (use gerrit), 2017/05/25 13:24:28]

!label.empty() might be slightly more readable

[chiniforooshan, 2017/05/26 15:46:31]

Done.

+  agent_.set_connection_error_handler(base::BindRepeating(
+      &AgentRegistry::Entry::OnConnectionError, base::Unretained(this)));
+}
+
+AgentRegistry::Entry::~Entry() {}

[oystein (OOO til 10th of July), 2017/05/25 17:16:08]

nit: = default;

[chiniforooshan, 2017/05/26 15:46:31]

Done.

+
+void AgentRegistry::Entry::SetDisconnectClosure(base::OnceClosure closure) {
+  DCHECK(closure_.is_null());

[oystein (OOO til 10th of July), 2017/05/25 17:16:08]

nit: "base/logging.h"

[chiniforooshan, 2017/05/26 15:46:31]

Done.

+  closure_ = std::move(closure);
+}
+
+bool AgentRegistry::Entry::RemoveDisconnectClosure() {
+  bool closure_was_set = !closure_.is_null();
+  closure_.Reset();
+  return closure_was_set;

[oystein (OOO til 10th of July), 2017/05/25 17:16:08]

just curious: why is this needing to be returned?

[chiniforooshan, 2017/05/26 15:46:31]

Coordinator will use disconnect closures to make sure that if it's waiting to
receive replies from all agents, and an agent disconnects before replying, it
does not wait for it forever. For example,

// Send a message to an agent.
agent->SOME_INTERFACE_METHOD_WITH_REPLY(..., base::Bind(&OnReply, ...));
// Set the disconnect closure.
agent->SetDisconnectClosure(base::Bind(&OnReply, ...));

Then, I believe it is technically possible that |OnReply| is invoked two times
if the agent replies and quickly disconnects, before the coordinator thread is
freed to process the reply task. So, |OnReply| should clear the disconnect
closure and bail out if it is already cleared, because that indicates that
|OnReply| was already invoked for this agent.

Does this make sense?

[oystein (OOO til 10th of July), 2017/05/26 17:07:43]

Yep absolutely, thanks for the explanation!

+}
+
+void AgentRegistry::Entry::OnConnectionError() {
+  if (!closure_.is_null())
+    std::move(closure_).Run();

[oystein (OOO til 10th of July), 2017/05/25 17:16:08]

This line confuses me :). Is this moving closure_ to a stack unique_ptr
basically? Maybe that should be a little bit more explicit.

[chiniforooshan, 2017/05/26 15:46:31]

It marks |closure_| as movable so that the version of Run that takes rvalue
reference can be selected. If I don't mark it as movable the Run that takes
const reference will be selected, which is for repeating callbacks, not once
callbacks.

[oystein (OOO til 10th of July), 2017/05/26 17:07:43]

Ouch, that's obscure. If there's no way to write this that makes this clearer,
that definitely needs a comment.

+  agent_registry_->UnregisterAgent(id_);
+}
+
+// static
+AgentRegistry* AgentRegistry::GetInstance() {
+  return g_agent_registry;
+}
+
+AgentRegistry::AgentRegistry() {
+  DCHECK(!g_agent_registry);
+  g_agent_registry = this;
+}
+
+AgentRegistry::~AgentRegistry() {
+  g_agent_registry = nullptr;

[Primiano Tucci (use gerrit), 2017/05/25 13:24:28]

maybe add a comment saying // for testing only (is it?), otherwise I'd argue
this is racy :)

[chiniforooshan, 2017/05/26 15:46:31]

Done.

+}
+
+void AgentRegistry::BindAgentRegistryRequest(
+    const service_manager::BindSourceInfo& source_info,
+    mojom::AgentRegistryRequest request) {
+  bindings_.AddBinding(this, std::move(request));
+}
+
+void AgentRegistry::SetAgentInitializationCallback(
+    const AgentInitializationCallback& callback) {
+  agent_initialization_callback_ = callback;
+  ForAllAgents(
+      [this](Entry* entry) { agent_initialization_callback_.Run(entry); });
+}
+
+void AgentRegistry::RemoveAgentInitializationCallback() {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  agent_initialization_callback_.Reset();
+}
+
+void AgentRegistry::RegisterAgent(mojom::AgentPtr agent,
+                                  const std::string& label,
+                                  mojom::TraceDataType type,
+                                  bool supports_explicit_clock_sync) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  auto id = next_agent_id_++;
+  auto entry = base::MakeUnique<Entry>(id, this, std::move(agent), label, type,
+                                       supports_explicit_clock_sync);
+  if (!agent_initialization_callback_.is_null())
+    agent_initialization_callback_.Run(entry.get());
+  auto result = agents_.insert(std::make_pair(id, std::move(entry)));
+  DCHECK(result.second);
+}
+
+void AgentRegistry::UnregisterAgent(size_t agent_id) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  size_t num_deleted = agents_.erase(agent_id);
+  DCHECK_EQ(1u, num_deleted);
+}
+
+}  // namespace tracing