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_forward.h"
+#include "base/sequenced_task_runner.h"
+
+namespace base {
+class TimeDelta;
+}
+
+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.

[Wez, 2014/12/09 00:51:58]

By acking, do you mean a completion callback?

Why would such a function be ACK but not provide a success/failure result?

[Łukasz Anforowicz, 2014/12/09 18:47:07]

I am not sure if I understand the question.

If by "provide [...] failure result" you mean "provide timeout result", then the
"why" answer is: it is easier to unit test a generic ReportAckOrTimeout
function, then timeout embedded inside HeartbeatSender or MinimumHeartbeatSender
(the timeout is in MinimumHeartbeatSender in patchset#1, adding unit tests
prompted extracting this functionality to a separate class).

If the above misunderstands your question, then maybe we can discuss futher by
looking at a concrete |function_that_acks| - a partially bound
HeartbeatSender::SendHostOfflineReason.  When SendHostOfflineReason calls
ack_callback, then the host-offline-reason has been sent to and accepted by the
bot and directory service.

BTW: Do you think I should also rename ack_callback to completion_callback in
HeartbeatSender::SendHostOfflineReason?

+//
+// 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(

[Wez, 2014/12/09 00:51:58]

This name doesn't seem very helpful/descriptive; this function actually calls a
supplied "callback" and then times-out if it doesn't "ack" within the specified
|timeout|, so call this e.g. CallAsyncMethodWithTimeout() or similar.

[Wez, 2014/12/09 00:51:58]

Why do you need this function at all? It seems it's only used to call the
HeartbeatSender function - why not make that indicate success/failure and add a
timeout there?

[Łukasz Anforowicz, 2014/12/09 18:47:07]

I find it easier to unit test a generic ReportAckOrTimeout, than the same
functionality hardwired inside HeartbeatSender or MinimumHeartbeatSupporter (in
patchset#1 the timeout was handled inside MinimumHeartbeatSupporter and
consequently there were no unit tests).

[Łukasz Anforowicz, 2014/12/09 18:47:07]

I can rename (if we agree that having a ReportAckOrTimeout functionality outside
of HeartbeatSender / MinimumHeartbeatSupporter is desirable).

+    const base::Callback<void(const base::Closure& ack_callback)>&
+        function_that_acks,

[Wez, 2014/12/09 00:51:58]

nit: typedef this to something like AsyncFunction?

[Łukasz Anforowicz, 2014/12/09 18:47:07]

I like that the reader can see the signature of function_that_acks right here,
without having to hunt down typedefs...  I'd rather leave it as is.

+    const base::TimeDelta& timeout,
+    scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner,

[Wez, 2014/12/09 00:51:58]

Why do you need this parameter, rather than using ThreadTaskRunnerHandle::Get()?

[Łukasz Anforowicz, 2014/12/09 18:47:07]

This way I can avoid unnecessarily constraining ReportAckOrTimeout to only be
called on the same task_runner as the runner used to invoke ack_callback.

[Wez, 2014/12/15 20:34:24]

It's already constrained to be the same thread by the implementation, though,
since you bind a WeakPtr to the ACK-or-timeout reporting class into the delayed
task that you post to |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_