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

remoting/host/ack_or_timeout_reporter.cc

+// 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.
+
+#include "remoting/host/ack_or_timeout_reporter.h"
+
+#include "base/bind.h"
+#include "base/location.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/sequence_checker.h"
+#include "base/time/time.h"
+
+namespace remoting {
+
+class AckOrTimeoutReporter : public base::RefCounted<AckOrTimeoutReporter> {

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

Why does this need to be ref-counted? The only thing that owns it is itself, and
it deletes itself on ack or timeout, so there's no ambiguity there, surely?

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

"The only thing that owns it is itself" is not true.  It is owned by 1) itself
and 2) the ReportAckOrTimeout method.  I think it needs to be owned from those 2
places: 
1) holding a reference only from self can end up pulling a rag from under
ReportAckOrTimeout (which needs the object while calling the Execute instance
method);  the rag can be pulled if function_that_acks ends up running a nested
task message pump.
2) holding a reference only from the ReportAckOrTimeout method means that the
AckOrTimeoutReporter object can be destroyed too early (i.e. causing callbacks
to become no-ops [via weak ptrs] too early).

I thought about holding a scoped_ptr in ReportAckOrTimeout function and
"Pass"-ing it to AckOrTimeoutReporter, but this cannot work - the pointer cannot
be simulteanously passed as an argument *and* at the same time used as "this" in
instance method invocation.

[Łukasz Anforowicz, 2014/12/10 05:11:09]

I've added a unit test where ack_callback is synchronously called by
function_that_acks.  In this case when ack_callback runs, the Execute instance
method is still on the callstack.  

When adding this case, I thought that it will help enforce proper behavior wrt
lifetime management.  After adding this case, I realized that the call to the
function_that_acks is the last statement of Execute method, so in theory it
might be ok to rcelease AckOrTimeoutReporter from within
AckOrTimeoutReporter::AckOrTimeout.  Still - the test case is useful anyway
(helps validate a valid, separate scenario), so I'll leave it in.

+ public:
+  AckOrTimeoutReporter(
+      scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner,
+      const base::Callback<void(AckOrTimeout)>& ack_or_timeout_callback);
+
+  void Execute(
+      const base::Callback<void(const base::Closure& ack)>& function_that_acks,
+      const base::TimeDelta& timeout);
+
+ private:
+  friend class base::RefCounted<AckOrTimeoutReporter>;
+  ~AckOrTimeoutReporter();
+
+  void OnAckOrTimeout(AckOrTimeout ack_or_timeout);
+
+  scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner_;
+  base::Callback<void(AckOrTimeout)> ack_or_timeout_callback_;
+  base::WeakPtrFactory<AckOrTimeoutReporter> weak_ptr_factory_;
+  scoped_refptr<AckOrTimeoutReporter> self_;
+  base::SequenceChecker sequence_checker_;
+
+  DISALLOW_COPY_AND_ASSIGN(AckOrTimeoutReporter);
+};
+
+AckOrTimeoutReporter::AckOrTimeoutReporter(
+    scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner,
+    const base::Callback<void(AckOrTimeout)>& ack_or_timeout_callback)
+    : sequenced_task_runner_(sequenced_task_runner),
+      ack_or_timeout_callback_(ack_or_timeout_callback),
+      weak_ptr_factory_(this),
+      self_(this) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());

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

This check is a no-op; the sequence checker binds to the thread on which it is
created.

Also, why do you have a SequenceChecker *and* a SequencedTaskRunner? If you hold
a TaskRunner you can just check whether the current thread belongs to it via
RunsTasksOnCurrentThread().

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

Good point.  This was written this way before I was requiring a
SequencedTaskRunner to be passed in.  I'll switch to checking via task_runner
(if we reach an agreement that using an explicit SequencedTaskRunner is a good
idea - this is being discussed in other comments).

+}
+
+AckOrTimeoutReporter::~AckOrTimeoutReporter() {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());

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

SequenceChecker will do this in its own dtor.

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

I'll keep this (because based on another comment I will be probably switching
back to doing the checks explicitly / via task_runner).

+}
+
+void AckOrTimeoutReporter::Execute(
+    const base::Callback<void(const base::Closure& ack)>& function_that_acks,
+    const base::TimeDelta& timeout) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+
+  base::Closure timeout_callback =
+      base::Bind(&AckOrTimeoutReporter::OnAckOrTimeout,
+                 weak_ptr_factory_.GetWeakPtr(), AckOrTimeout::Timeout);
+  base::Closure ack_callback =
+      base::Bind(&AckOrTimeoutReporter::OnAckOrTimeout,
+                 weak_ptr_factory_.GetWeakPtr(), AckOrTimeout::Ack);
+
+  sequenced_task_runner_->PostDelayedTask(FROM_HERE, timeout_callback, timeout);

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

Couldn't you use a OneShotTimer here rather than requiring the sequenced task
runner and the WeakPtrFactory below?

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

I don't understand the proposal.

- Requirement #1: AckOrTimeoutReporter needs to cancel the ack_callback when
timeout_callback happens (and the other way round as well).
- Requirement #2: AckOrTimeoutReporter should be released from memory after the
external ack_or_timeout_callback is called.  
- Design decision #3: ack_callback and timeout_callback need to happen in
sequence (an alternative design decision would be to enforce thread safety in
some other way)
- Requirement #4: Make this easy to unit tests.

I don't understand how a OneShotTimer can achieve #1 without requiring
long-lived memory with a flag that flags whether a callback already happened
(violating #2).  I guess requirement #2 could be relaxed to say only
|ack_or_timeout_callback| needs to be released.

OneShotTimer will find task runner via TLS, but there is no guarantee that this
is the same task runner that will be used for calling ack_callback (in fact,
with the current code, it is perfectly fine to call ReportAckOrTimeout from
*another* task runner).  We could ask the caller to only call ReportAckOrTimeout
from the same task_runner that will be sued to call ack_callback, but 1)
explicit sequenced_task_runner seems to be a cleaner, more direct way of
explaining this contract and 2) explicit sequenced_task_runner doesn't
unnecessarily constraint on what task runner the ReportAckOrTimeout function can
be called.

Having an explicit timer-like object also helps with dependency injection for
unit testing (#4).  I guess I could always mock a timer stored in TLS, but it
seems like more trouble than just explicitly passing a mock sequence task
runner.

+  function_that_acks.Run(ack_callback);
+}
+
+void AckOrTimeoutReporter::OnAckOrTimeout(AckOrTimeout ack_or_timeout) {
+  DCHECK(sequence_checker_.CalledOnValidSequencedThread());
+
+  // Make sure |ack_callback| and |timeout_callback| created
+  // in |Execute| method become no-ops that are safe to be invoked
+  // after |this| is no longer alive.
+  // This is done as the very first thing here, to make sure
+  // |ack_callback| and |timeout_callback| won't be called
+  // again no matter what happens next (i.e. as a consequence
+  // of |ack_or_timeout_callback_|.

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

How could anything that that callback does possibly trigger the others?

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

i.e. it could start a nested TaskRunner pump.

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

That's specifically disallowed in Chromium production code; it's only do-able in
tests AFAIK.

+  weak_ptr_factory_.InvalidateWeakPtrs();
+
+  ack_or_timeout_callback_.Run(ack_or_timeout);
+  self_ = nullptr;
+}
+
+void ReportAckOrTimeout(
+    const base::Callback<void(const base::Closure&)>& function_that_acks,
+    const base::TimeDelta& timeout,
+    scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner,
+    const base::Callback<void(AckOrTimeout)>& ack_or_timeout_callback) {
+  scoped_refptr<AckOrTimeoutReporter> reporter(
+      new AckOrTimeoutReporter(sequenced_task_runner, ack_or_timeout_callback));
+  reporter->Execute(function_that_acks, timeout);
+}
+
+}  // namespace remoting