[Chromoting] PromisedRaisable in Android client

remoting/android/java/src/org/chromium/chromoting/Event.java

 // Copyright 2016 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.
 
 package org.chromium.chromoting;
 
 import java.util.HashSet;
 
 /**
  * A thread-safe event queue which provides both {@link #add} and {@link #remove} functions with
- * O(log(n)) time complexity, and a {@link raise} function in the derived class
+ * O(log(n)) time complexity, and a {@link #raise} function in the derived class
  * {@link Event.Raisable} to execute all queued callbacks.
  *
  * @param <ParamT> The parameter used in {@link ParameterRunnable} callback.
  */
 public class Event<ParamT> {
     /** A runnable with parameter. */
     public static interface ParameterRunnable<ParamT> {
         void run(ParamT p);
     }
 
     /** A callback with parameter. */
     public static interface ParameterCallback<ReturnT, ParamT> {
         ReturnT run(ParamT p);
     }
 
     /**
-     * An event provider version of {@link Event} implementation, provides {@link raise} function to
-     * execute appended {@link ParameterRunnable}, and {@link clear} function to clear all appended
-     * callbacks.
+     * An event provider version of {@link Event} implementation, provides {@link #raise} function
+     * to execute appended {@link ParameterRunnable}, and {@link clear} function to clear all
+     * appended callbacks.
      */
-    public static final class Raisable<ParamT> extends Event<ParamT> {
+    public static class Raisable<ParamT> extends Event<ParamT> {
         /** Clears all appended callbacks */
         public void clear() {
             synchronized (mSet) {
                 mSet.clear();
             }
         }
 
         /**
          * Executes all queued {@link ParameterRunnable} with |parameter|, returns an integer of
          * total callbacks executed. Note, if an 'add' function call is executing concurrently
@@ skipping 14 matching lines @@
 
         /** Executes |obj| as ParameterRunnable<ParamT> with |parameter| as ParamT. */
         @SuppressWarnings("unchecked")
         private void execute(Object obj, ParamT parameter) {
             ParameterRunnable<ParamT> runnable = (ParameterRunnable<ParamT>) obj;
             runnable.run(parameter);
         }
     }
 
     /**
+     * An {@link Raisable} which always executes the newly added {@link ParameterRunnable} with the

[Lambros, 2016/10/05 19:30:04]

s/An/A/

[Hzj_jie, 2016/10/06 00:08:09]

Done.

+     * parameter sent to the last {@link #raise} function call. If the event has not been raised,
+     * this class has a consistent behavior as {@link Raisable}.

[Lambros, 2016/10/05 19:30:03]

..has consistent behavior with..
or
..behaves the same as..

[Hzj_jie, 2016/10/06 00:08:09]

Done.

+     * <p> This class is useful for some one-time events, such as RenderStub.onClientSizeChanged().

[Lambros, 2016/10/05 19:30:04]

I might be mistaken, but is it common style to use <p> for paragraphs instead of
just using a blank line? <p> looks a bit ugly to me, but if it's common
elsewhere, I guess we can stick with it?

[Hzj_jie, 2016/10/06 00:08:09]

AFAICT, without <p> or <br>, the newline character will simply be ignored, and
replaced by a space in the final html output.
I intentionally separate the comments into several paragraphs. I may also use
<ul> and <li> here, which displays the sentences as a list.

+     * A later attached runnable will never be able to get the event.
+     * <p> The {@link ParameterRunnable} will be executed in the thread which {@link #add} function

[Lambros, 2016/10/05 19:30:04]

..thread which calls {@link #add}..
or
..thread in which {@link #add} is called..

[Yuwei, 2016/10/05 20:50:34]

I think to be consistent it could be better to run all callbacks on the same
thread. Maybe the thread where raise() is first called or the constructor is
called.

Also since now all Events are being used on the UI thread, we may simplify these
problems and make Event single-threaded.

[Hzj_jie, 2016/10/06 00:08:09]

Done.

[Hzj_jie, 2016/10/06 00:08:09]

Yes, but I would prefer to remove all the synchronizations in a single change,
and update the comments. Before that, the Event class is designed to be
thread-safe. Writing a class with partial thread-safety looks weird to me.

[Yuwei, 2016/10/06 01:32:24]

Acknowledged.

+     * is called if the event has been raised before.

[Lambros, 2016/10/05 19:30:04]

You're saying that the implementation will call the Runnable in the "same
thread". But actually this implementation calls it *immediately*.
This will come as a surprise to those who are expecting Promise-like async
behavior. The Promise specification requires that the callback does not
immediately trigger:
https://promisesaplus.com/#point-34 (2.2.4)

We should either follow the same design choice here. Or we should make it clear
in the JavaDoc that the method may be called synchronously inside the add()
call. I think I would prefer the former, even though it means writing extra code
to post the task onto the current message-loop.

[joedow, 2016/10/05 19:46:43]

Agreed, I've used events which can either fire sync or async and they can be a
pain due to race conditions.  I'd prefer they are always async as well.

[Yuwei, 2016/10/05 20:50:34]

Yep. Running the callbacks in current task will have the risk of infinite
recursion if the callback raises the same event.

[Hzj_jie, 2016/10/06 00:08:09]

This should not happen, raise is only called from providers instead of
listeners.

The asynchronous callback is a good suggestion, I will update.

But I do find a potential issue. A posted task in current looper may change
mLastParameter by raising the event. So a newly attached runnable may not be
able to get the exact parameter before it attached. The test cases show the
difference.
And we also cannot simply record mLastParameter, it will impact the ordering of
parameters.
I do not see this could make any real trouble for now. But I would prefer to add
some comments to explain the issue here.

To avoid the edge case, the add is called on a thread without looper, I will
still execute the runnable immediately. This won't happen in our logic, since we
are always running in the main looper. But some thirdparty threads may not have
a looper, crashing or ignoring in these threads seems not correct to me.

[Yuwei, 2016/10/06 01:32:24]

Well, you code doesn't disallow circular event dependency :P
IIUC your concern is, when A,B is raised:

1. You get B,B if you capture `this`
2. you get B,A if you capture mLastParameter

Right? I'm okay with getting B,B since we only care about the latest value. Or
you can use a boolean to avoid double feeding the listener.

[Hzj_jie, 2016/10/06 19:01:34]

I think that's a totally incorrect pattern. It looks weird if a component listen
to another event just for raising its own event. The listener should listen to
the original event instead.
Yes, reordering is definitely not acceptable, since most listeners care about
the latest value. I cannot quite tell where is the right place to add the
boolean value. But another scenario may be listeners care about how many times
the event fired, say, a statistic component listens to onCanvasRendered event to
calculate the FPS. The data is not important, but the number of times is. So
current solution can satisfy both scenarios.

+     * <p> The {@link ParameterRunnable} may be executed twice on different threads with exactly

[Lambros, 2016/10/05 19:30:04]

Personally, I wouldn't make this a separate paragraph. I would continue the
previous paragraph with:
"Note that it may be executed twice..."

If you say "Note that..." or "However, ...", you are clarifying that this is an
exception to the claim that the Runnable only triggers once. This means you can
remove the final sentence: "Usually.."

[Hzj_jie, 2016/10/06 00:08:09]

Done.

+     * same parameter. Usually this is not an issue, but consumers need to aware.

[Lambros, 2016/10/05 19:30:04]

..to be aware.
But you can remove this anyway.

[Hzj_jie, 2016/10/06 00:08:09]

Done.

+     */
+    public static final class ReproducibleRaisable<ParamT> extends Raisable<ParamT> {

[joedow, 2016/10/05 19:46:43]

The name ReproducibleRaisable doesn't tell me that this is going to fire if it
has already been called in the past.  I'd think about naming this differently so
someone scanning the code can get a better idea of what this class does.

[Hzj_jie, 2016/10/06 00:08:09]

Changed to PromisedRaisable.

+        private boolean mRaised;
+        private ParamT mLastParameter;
+
+        @Override
+        public Object add(ParameterRunnable<ParamT> runnable) {
+            Object result = super.add(runnable);
+            if (result != null) {
+                synchronized (mSet) {
+                    if (mRaised) {

[Yuwei, 2016/10/05 20:50:34]

Could you do the check without the boolean, say checking whether mLastParameter
is not null?

[Hzj_jie, 2016/10/06 00:08:09]

No, not a good idea, we cannot ensure a null-able parameter is always used in an
Event. (Indeed we have several events with Void parameter type.)

[Yuwei, 2016/10/06 01:32:24]

Ah. I think you are right.

+                        runnable.run(mLastParameter);
+                    }
+                }
+            }
+            return result;
+        }
+
+        @Override
+        public int raise(ParamT parameter) {
+            synchronized (mSet) {
+                mRaised = true;
+                mLastParameter = parameter;
+            }
+            return super.raise(parameter);
+        }
+    }
+
+    /**
      * A self removable {@link ParameterRunner}, uses a boolean {@link ParameterCallback} to decide
      * whether removes self from {@link Event} or not.
      */
     private static final class SelfRemovableParameterRunnable<ParamT>
             implements ParameterRunnable<ParamT> {
         private final ParameterCallback<Boolean, ParamT> mCallback;
         private final Event<ParamT> mOwner;
 
         // This lock is used to make sure mEvent is correctly set before remove in run function.
         // i.e. mOwner.add and assigment of mEvent need to be atomic.
@@ skipping 90 matching lines @@
 
     /**
      * Returns true if there is no runnable attached to current instance.
      */
     public boolean isEmpty() {
         synchronized (mSet) {
             return mSet.isEmpty();
         }
     }
 }