Reporting of policy errors via host-offline-reason: part 3

remoting/host/ack_or_timeout_reporter.h

+// Copyright 2014 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 REMOTING_HOST_ACK_OR_TIMEOUT_REPORTER_H_
+#define REMOTING_HOST_ACK_OR_TIMEOUT_REPORTER_H_
+
+#include "base/callback.h"
+#include "base/sequenced_task_runner.h"
+#include "base/time/time.h"
+
+namespace remoting {
+
+enum AckOrTimeout {
+  Ack,
+  Timeout
+};
+
+// ReportAckOrTimeout will call |ack_or_timeout_callback|, when either
+// |ack_callback| is called or when |timeout| has been reached.  This
+// is helpful to add timeout functionality to a function that supports
+// acking, but doesn't support timeouts.
+//
+// As a consequence of using WeakPtr as an implementation detail,
+// ReportAckOrTimeout requires its caller to make sure that:
+// - ReportAckOfTimeout is called on |sequenced_task_runner|
+// - If called, then |ack_callback| will be called on |sequenced_task_runner|
+//
+// ReportAckOrTimeout guarantees that:
+// - |function_that_acks| will be called synchronously (i.e. caller
+//   can destroy |function_that_acks| and/or its base::Unretained
+//   arguments as soon as ReportAckOrTimeout returns).
+// - |ack_or_timeout_callback| will be called exactly once.
+// - |ack_or_timeout_callback| will be called via |sequenced_task_runner|.
+// - All references (i.e. ref-counted arguments) to |ack_or_timeout_callback|
+//   will be dropped after it is called.
+void ReportAckOrTimeout(
+    const base::Callback<void(const base::Closure& ack_callback)>
+                                                   function_that_acks,

[Lambros, 2014/11/19 02:06:17]

I'm having a little conceptual trouble with this. Why does |function_that_acks|
have to be a function that takes a parameter? Why can't it be a fully-bound
Closure itself?

You have some function that, when called, promises to call some other function
some time in the future. That other function might be related to the first
function in many different ways. It might be the first and only parameter (which
is what this code assumes). It might be the 29th parameter. It might be a member
variable of some object that is passed as a parameter. And so on...

In fact, why does |function_that_acks| need to be passed in at all? All we do is
Run() it, then throw it away. The caller could just as well do that itself?

[Łukasz Anforowicz, 2014/11/19 21:44:24]

I don't understand what you mean by "Why can't it be a fully-bound Closure
itself".  If you want to separate act of invoking |function_that_acks| from the
act of registering an |ack_callback| then I think this won't work - i.e.
|function_that_acks| might ack before ReportAckOrTimeout registers an
|ack_callback|.
The complaints about |ack_callback| being 1st or 29th parameter or a member
field or listener pattern are valid, but I think they don't belong here.  Such
complains apply to any environment that supports partially bound functions,
regardless if it is Haskell, Chromium's Callback or something else.  If
Chromium's Callbacks don't easily support partially binding of middle arguments,
then we should fix Chromium's Callbacks (when such partial binding is actually
needed).  

Anyway - I am not sure why the 1st vs 29th parameter is brought up here, because
in case of HeartbeatSender things work just fine, because |ack_callback| is the
last parameter.  We are extracting ReportAckOrTimeout from
MinimumHeartbeatSender to make it testable, and not necessarily to make it fully
generic, broadly applicable function (which it is, as long as Chromium's
Callbacks machinery is easy to wrap parameters in a different order).
We do more than run |function_that_acks| and throw it away :-)  We run
|function_that_acks| while at the same time providing an |ack_callback|
controlled by internals of AckOrTimeoutReporter.

+    const base::TimeDelta& timeout,
+    scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner,
+    const base::Callback<void(AckOrTimeout ack_or_timeout)>
+                                                    ack_or_timeout_callback);
+
+}  // namespace remoting
+
+#endif  // REMOTING_HOST_ACK_OR_TIMEOUT_REPORTER_H_