Restart of ceee_broker on crash.

ceee/ie/broker/broker_rpc_client.cc

Index: ceee/ie/broker/broker_rpc_client.cc
===================================================================
--- ceee/ie/broker/broker_rpc_client.cc	(revision 67655)
+++ ceee/ie/broker/broker_rpc_client.cc	(working copy)
@@ -9,17 +9,61 @@
 #include <atlbase.h>
 #include "base/lock.h"
 #include "base/logging.h"
+#include "base/tuple.h"
 #include "base/win/scoped_comptr.h"
 #include "broker_lib.h"  // NOLINT
 #include "broker_rpc_lib.h"  // NOLINT
 #include "ceee/common/com_utils.h"
 #include "ceee/ie/broker/broker_rpc_utils.h"
 
-BrokerRpcClient::BrokerRpcClient() : context_(0), binding_handle_(NULL) {
+
+namespace {
+
+void LogRpcException(const char* str, unsigned int exception_code) {
+  LOG(ERROR) << str << com::LogWe(exception_code);
 }
 
-BrokerRpcClient::~BrokerRpcClient() {
-  Disconnect();
+HRESULT BindRpc(std::wstring endpoint, RPC_BINDING_HANDLE* binding_handle) {

[Sigurður Ásgeirsson, 2010/12/01 14:23:54]

const & for endpoint.

[Vitaly Buka corp, 2010/12/01 19:06:04]

I need modifiable string to avoid const_cast
On 2010/12/01 14:23:54, Ruðrugis wrote:

+  DCHECK(binding_handle);

[Sigurður Ásgeirsson, 2010/12/01 14:23:54]

we typically do a NULL check explicitly e.g. 
DCHECK(binding_handle != NULL)

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+  std::wstring protocol = kRpcProtocol;
+  DCHECK(!protocol.empty());

[MAD, 2010/12/01 16:35:23]

Why do we need to put this in an std::wstring?
And since kRpcProtocol is an invariant, do we really need to test it every time?

[Vitaly Buka corp, 2010/12/01 19:06:04]

RpcStringBindingCompose need non cost. So I can avoid wstring by using
const_cast

On 2010/12/01 16:35:23, MAD wrote:

+  DCHECK(!endpoint.empty());
+  if (protocol.empty() || endpoint.empty() || binding_handle == NULL)
+    return E_INVALIDARG;
+
+  RPC_BINDING_HANDLE tmp_binding_handle = NULL;
+
+  // TODO(vitalybuka@google.com): There's no guarantee (aside from name
+  // uniqueness) that it will connect to an endpoint created by the same user.
+  // Hint: The missing invocation is RpcBindingSetAuthInfoEx.
+  LOG(INFO) << "Connecting to RPC server. Endpoint: " << endpoint;

[MAD, 2010/12/01 16:35:23]

We should start using VLOG(n)...

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+  RPC_WSTR string_binding = NULL;
+  // Create binding string with given protocol and end point.

[MAD, 2010/12/01 16:35:23]

Actually, protocol is not given it's hard coded...

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+  RPC_STATUS status = ::RpcStringBindingCompose(
+      NULL,
+      reinterpret_cast<RPC_WSTR>(&protocol[0]),
+      NULL,
+      reinterpret_cast<RPC_WSTR>(&endpoint[0]),
+      NULL,
+      &string_binding);
+  LOG_IF(ERROR, RPC_S_OK != status) <<
+      "Failed to compose binding string. RPC_STATUS=0x" << com::LogWe(status);

[MAD, 2010/12/01 16:35:23]

Could also use LogRpcException in an else clause below.

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+
+  if (RPC_S_OK == status) {
+    // Create binding from just generated binding string. Binding handle should

[MAD, 2010/12/01 16:35:23]

should used -> should be used

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+    // used for PRC calls.
+    status = ::RpcBindingFromStringBinding(string_binding, &tmp_binding_handle);
+    LOG_IF(ERROR, RPC_S_OK != status) <<
+        "Failed to bind. RPC_STATUS=0x" << com::LogWe(status);
+    ::RpcStringFree(&string_binding);
+    if (RPC_S_OK == status) {
+      LOG(INFO) << "RPC client is connected. Endpoint: " << endpoint;
+      *binding_handle = tmp_binding_handle;
+      return S_OK;
+    }
+  }
+  ::RpcBindingFree(binding_handle);

[MAD, 2010/12/01 16:35:23]

What if :RpcStringBindingCompose() failed? What are we freeing here?

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+  return RPC_E_FAULT;
 }
 
 int HandleRpcException(unsigned int rpc_exception_code) {
@@ -38,10 +82,18 @@
   return EXCEPTION_EXECUTE_HANDLER;
 }
 
-static void LogRpcException(const char* str, unsigned int exception_code) {
-  LOG(ERROR) << str << com::LogWe(exception_code);
+}  // namespace
+
+BrokerRpcClient::BrokerRpcClient(bool allow_restarts)
+    : context_(0),
+      binding_handle_(NULL),
+      allow_restarts_(allow_restarts) {
 }
 
+BrokerRpcClient::~BrokerRpcClient() {
+  Disconnect();
+}
+
 void BrokerRpcClient::LockContext() {
   RpcTryExcept {
     context_ = BrokerRpcClient_Connect(binding_handle_);
@@ -58,56 +110,33 @@
   } RpcEndExcept
 }
 
+HRESULT BrokerRpcClient::StartServer(IUnknown** server) {
+  base::win::ScopedComPtr<IUnknown> broker;
+  // TODO(vitalybuka@google.com): Start broker without COM after the last
+  // COM interface is removed.
+  HRESULT hr = broker.CreateInstance(CLSID_CeeeBroker);
+  LOG_IF(ERROR, FAILED(hr)) << "Failed to create broker. " << com::LogHr(hr);
+  if (FAILED(hr))
+    return hr;
+  *server = broker.Detach();
+  return S_OK;
+}
+
 HRESULT BrokerRpcClient::Connect(bool start_server) {
   if (is_connected())
     return S_OK;
 
   // Keep alive until RPC is connected.
-  base::win::ScopedComPtr<ICeeeBrokerRegistrar> broker;
+  base::win::ScopedComPtr<IUnknown> broker;
   if (start_server) {
-    // TODO(vitalybuka@google.com): Start broker without COM after the last
-    // COM interface is removed.
-    HRESULT hr = broker.CreateInstance(CLSID_CeeeBroker);
-    LOG_IF(ERROR, FAILED(hr)) << "Failed to create broker. " << com::LogHr(hr);
+    HRESULT hr = StartServer(broker.Receive());
     if (FAILED(hr))
       return hr;
   }
 
-  std::wstring end_point = GetRpcEndPointAddress();
-  std::wstring protocol = kRpcProtocol;
-  DCHECK(!protocol.empty());
-  DCHECK(!end_point.empty());
-  if (protocol.empty() || end_point.empty())
-    return RPC_E_FAULT;
+  if (SUCCEEDED(BindRpc(GetRpcEndpointAddress(), &binding_handle_)))
+    LockContext();
 
-  // TODO(vitalybuka@google.com): There's no guarantee (aside from name
-  // uniqueness) that it will connect to an endpoint created by the same user.
-  // Hint: The missing invocation is RpcBindingSetAuthInfoEx.
-  LOG(INFO) << "Connecting to RPC server. Endpoint: " << end_point;
-  RPC_WSTR string_binding = NULL;
-  // Create binding string with given protocol and end point.
-  RPC_STATUS status = ::RpcStringBindingCompose(
-      NULL,
-      reinterpret_cast<RPC_WSTR>(&protocol[0]),
-      NULL,
-      reinterpret_cast<RPC_WSTR>(&end_point[0]),
-      NULL,
-      &string_binding);
-  LOG_IF(ERROR, RPC_S_OK != status) <<
-      "Failed to compose binding string. RPC_STATUS=0x" << com::LogWe(status);
-
-  if (RPC_S_OK == status) {
-    // Create binding from just generated binding string. Binding handle should
-    // used for PRC calls.
-    status = ::RpcBindingFromStringBinding(string_binding, &binding_handle_);
-    LOG_IF(ERROR, RPC_S_OK != status) <<
-        "Failed to bind. RPC_STATUS=0x" << com::LogWe(status);
-    ::RpcStringFree(&string_binding);
-    if (RPC_S_OK == status) {
-      LOG(INFO) << "RPC client is connected. Endpoint: " << end_point;
-      LockContext();
-    }
-  }
   if (!is_connected()) {
     Disconnect();
     return RPC_E_FAULT;
@@ -125,25 +154,42 @@
   }
 }
 
-HRESULT BrokerRpcClient::FireEvent(const char* event_name,
-                                   const char* event_args) {
+template<class Function, class Params>
+HRESULT BrokerRpcClient::RunRpc(bool allow_restart,
+                                Function rpc_function,
+                                Params params) {

[Sigurður Ásgeirsson, 2010/12/01 14:23:54]

I would still suggest a const & here. As is, you always copy, even for the
common case.

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+  DCHECK(rpc_function);
+  if (!is_connected())
+    return RPC_E_FAULT;
   RpcTryExcept {
-    BrokerRpcClient_FireEvent(binding_handle_, event_name, event_args);
+    DispatchToFunction(rpc_function, params);
     return S_OK;
   } RpcExcept(HandleRpcException(RpcExceptionCode())) {
     LogRpcException("RPC error in FireEvent", RpcExceptionCode());

[MAD, 2010/12/01 16:35:23]

FireEvent -> Proper function name...

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

+
+    if (allow_restart &&
+        RPC_S_OK != ::RpcMgmtIsServerListening(binding_handle_)) {
+      Disconnect();
+      if (SUCCEEDED(Connect(true))) {
+        params.a = binding_handle_;

[Sigurður Ásgeirsson, 2010/12/01 14:23:54]

this is hopefully the less common case, so here you'd be forgiven to create a
second tuple and copy the args.

[MAD, 2010/12/01 16:35:23]

I wish there was a way to validate that params.a is ALWAYS a binding_handle_...
What if we add a new BrokerRpcClient_XXX function that doesn't use the handle as
the first param?

[Vitaly Buka corp, 2010/12/01 19:06:04]

Done.

[Vitaly Buka corp, 2010/12/01 19:06:04]

MakeRefTuple allows to remove this assignment.

I don't see much in this sense. 
On 2010/12/01 16:35:23, MAD wrote:

[MAD, 2010/12/01 19:59:40]

Ho, cool... Much better :-)

+        return RunRpc(false, rpc_function, params);

[MAD, 2010/12/01 16:35:23]

We should find a way to avoid infinite recursiveness here... What if it's our
call that kills the RPC server?

[Vitaly Buka corp, 2010/12/01 19:06:04]

It is impossible because first arg is 'false'.
On 2010/12/01 16:35:23, MAD wrote:

[MAD, 2010/12/01 19:59:40]

D'ho! I didn't read correctly... Sorry about that... :-)

+      }
+    }
     return RPC_E_FAULT;
   } RpcEndExcept
 }
 
+HRESULT BrokerRpcClient::FireEvent(const char* event_name,
+                                   const char* event_args) {
+  return RunRpc(allow_restarts_,
+                &BrokerRpcClient_FireEvent,
+                MakeTuple(binding_handle_, context_, event_name, event_args));
+}
+
 HRESULT BrokerRpcClient::SendUmaHistogramTimes(const char* name, int sample) {
-  RpcTryExcept {
-    BrokerRpcClient_SendUmaHistogramTimes(binding_handle_, name, sample);
-    return S_OK;
-  } RpcExcept(HandleRpcException(RpcExceptionCode())) {
-    LogRpcException("RPC error in SendUmaHistogramTimes", RpcExceptionCode());
-    return RPC_E_FAULT;
-  } RpcEndExcept
+  return RunRpc(allow_restarts_,
+                &BrokerRpcClient_SendUmaHistogramTimes,
+                MakeTuple(binding_handle_, name, sample));
 }
 
 HRESULT BrokerRpcClient::SendUmaHistogramData(const char* name,
@@ -151,12 +197,8 @@
                                               int min,
                                               int max,
                                               int bucket_count) {
-  RpcTryExcept {
-    BrokerRpcClient_SendUmaHistogramData(
-        binding_handle_, name, sample, min, max, bucket_count);
-    return S_OK;
-  } RpcExcept(HandleRpcException(RpcExceptionCode())) {
-    LogRpcException("RPC error in SendUmaHistogramData", RpcExceptionCode());
-    return RPC_E_FAULT;
-  } RpcEndExcept
+  return RunRpc(allow_restarts_,
+                &BrokerRpcClient_SendUmaHistogramData,
+                MakeTuple(binding_handle_, name, sample, min, max,
+                          bucket_count));
 }