Add Event and EventTest

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 complex, and a {@link raise} function in Executor inheritance to execute all

[Lambros, 2016/05/27 22:26:06]

s/complex/complexity/
s/Executor inheritance/the derived class {@link Event.Executor}/

[Hzj_jie, 2016/05/28 00:32:06]

Done.

+ * queued object.

[Lambros, 2016/05/27 22:26:06]

s/object/objects/

[Hzj_jie, 2016/05/28 00:32:07]

Done.

+ *
+ * @param <ParamType> The parameter used in {@link ParameterRunnable} callback.

[Lambros, 2016/05/27 22:26:06]

We should probably comply with the Google style guide for naming type
parameters:
https://google.github.io/styleguide/javaguide.html#s5.2.8-type-variable-names

(I don't think Chromium/Android style says anything about this)

[Hzj_jie, 2016/05/28 00:32:06]

Done.

+ */
+public class Event<ParamType> {
+    /**
+     * A runnable with parameter.

[Lambros, 2016/05/27 22:26:06]

Elsewhere, we've formatted single-line JavaDoc as
/** Foo bar. */
Do the same here for consistency?

[Hzj_jie, 2016/05/28 00:32:07]

I did not see a coding style for this, but yes, we should keep consistency.
Updated.

+     */
+    public static interface ParameterRunnable<ParamType> {
+        void run(ParamType p);
+    }
+
+    /**
+     * A callback with parameter.
+     */
+    public static interface ParameterCallback<ReturnType, ParamType> {
+        ReturnType run(ParamType p);
+    }
+
+    /**
+     * An event provider version of {@link Event} implementation, provides {@link raise} function to
+     * execute appended {@link ParameterRunnable}.
+     */
+    public static final class Executor<ParamType> extends Event<ParamType> {
+        public void clear() {

[Lambros, 2016/05/27 22:26:06]

Need JavaDoc for public methods (linter will complain, probably).

[Hzj_jie, 2016/05/28 00:32:07]

Done.

+            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
+         * with the 'raise' function call, the newly added object may not be executed.
+         */
+        public int raise(ParamType parameter) {
+            Object[] array;
+            synchronized (mSet) {
+                array = mSet.toArray();
+            }
+            int count = 0;
+            for (Object obj : array) {
+                execute(obj, parameter);
+                count++;
+            }
+            return count;
+        }
+
+        /**
+         * Executes |obj| as ParameterRunnable<ParamType> with |parameter| as ParamType.
+         */
+        @SuppressWarnings("unchecked")

[Lambros, 2016/05/27 22:26:06]

I'm not sure about this. Android style guide:
https://source.android.com/source/code-style.html#use-standard-java-annotations
says: @SuppressWarning should only be used when it is impossible to eliminate a
warning.
But this warning could be eliminated by using ParameterRunnable<ParamType>
instead of Object.
The downside is that the calling code has to declare the type, which is quite
long, reducing readability.
I would suggest that we follow the style guide, grit our teeth, and accept the
readability hit (though we gain some compile-time type checking). But I'll leave
it to your judgement.

[Hzj_jie, 2016/05/28 00:32:06]

Oops, I have tried several ways, but looks like we cannot bypass this cast. i.e.
we cannot use mSet.toArray(T[]), since we cannot create an array with type
ParameterRunnable<ParamT>, Java generic array creation error will be triggered.
And we cannot pass Object directly to ParameterRunnable<ParamT>, we still need
to cast. And since ParameterRunnable<?> does not necessary equal to
ParameterRunnable<ParamT>, we cannot even use instanceof operator to do the
check.
Do you have a better solution?

+        private void execute(Object obj, ParamType parameter) {
+            ParameterRunnable<ParamType> runnable = (ParameterRunnable<ParamType>) obj;
+            runnable.run(parameter);
+        }
+    }
+
+    /**
+     * A self removable {@link ParameterRunner}, uses a boolean {@link ParameterCallback} to decide
+     * whether removes self from {@link Event} or not.
+     */
+    private static class SelfRemovableParameterRunnable<ParamType>
+            implements ParameterRunnable<ParamType> {
+        private final ParameterCallback<Boolean, ParamType> mCallback;
+        private final Event<ParamType> 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.
+        private final Object mLock;
+        private final Object mEvent;
+
+        private SelfRemovableParameterRunnable(Event<ParamType> owner,
+                                               ParameterCallback<Boolean, ParamType> callback) {
+            Preconditions.notNull(callback);
+            mCallback = callback;
+            mOwner = owner;
+            mLock = new Object();
+            synchronized (mLock) {
+                mEvent = mOwner.add(this);

[Lambros, 2016/05/27 22:26:06]

Please verify that FindBugs is happy with leaking 'this' out of ctor.

It still feels very scary to me. Another thread might 'see' that the
Event contains an incompletely-constructed object.

(GN option: run_findbugs = true)

[Hzj_jie, 2016/05/28 00:32:06]

No, findbugs does not raise any issue. And synchronized here makes sure mOwner /
mLock -- which are two variables used in a different thread -- safe. So I think
it's safe.

+            }
+            Preconditions.notNull(mEvent);
+        }
+
+        public final void run(ParamType p) {
+            synchronized (mLock) {
+                if (mOwner.contains(mEvent)) {
+                    if (!mCallback.run(p)) {

[Lambros, 2016/05/31 17:50:35]

I know the CL has landed now, but I see a couple of problems with this:

1) We may be holding onto mLock for too long. The callback can contain
arbitrary code, which (in theory) could include a re-entrant call to
this same method. This would re-acquire the lock (so the lock would be
acquired twice). We would probably end up removing mEvent twice,
triggering the precondition failure.

2) Some other thread (or the same re-entrant thread) might remove the
object from the event (by calling Event.remove() or Event.clear()),
between checking mOwner.contains() and calling mOwner.remove().

Maybe remove the Preconditions check?

+                        Preconditions.isTrue(mOwner.remove(mEvent));
+                    }
+                }
+            }
+        }
+    }
+
+    protected final HashSet<ParameterRunnable<ParamType>> mSet;
+
+    public Event() {
+        mSet = new HashSet<>();
+    }
+
+    /**
+     * Adds a {@link ParameterRunnable} object into current instance, returns an object which can
+     * be used to {@link remove} the runnable from this instance. If |runnable| is null or it has
+     * been added to this instance already, this function returns null.
+     */
+    public Object add(ParameterRunnable<ParamType> runnable) {
+        if (runnable == null) {
+            return null;
+        }
+        synchronized (mSet) {
+            if (mSet.add(runnable)) {
+                return runnable;
+            }
+        }
+        return null;
+    }
+
+    /**
+     * Adds a self removable {@link ParameterRunnable} object into current instance, the runnable

[Lambros, 2016/05/27 22:26:06]

Grammar nit:
This is 2 sentences, so:
Either s/,/;/
Or s/, the/. The/

[Hzj_jie, 2016/05/28 00:32:06]

Done.

+     * will remove itself from {@link Event} instance when callback returns false.
+     */
+    public void addSelfRemovable(ParameterCallback<Boolean, ParamType> callback) {
+        Preconditions.notNull(callback);
+        // SelfRemovableParameterRunner is self-contained, i.e. consumers do not need to have a

[Lambros, 2016/05/27 22:26:07]

Blank line before comment.

[Hzj_jie, 2016/05/28 00:32:06]

Done.

+        // reference of this instance, but all the logic is in new function.
+        new SelfRemovableParameterRunnable<ParamType>(this, callback);
+    }
+
+    /**
+     * Removes an object that was previously returned by {@link add}. Returns false if the object
+     * is not in the event queue, or not returned by {@link add} function.
+     */
+    public boolean remove(Object obj) {
+        if (obj == null) {

[Lambros, 2016/05/27 22:26:06]

Maybe remove this test?

[Hzj_jie, 2016/05/28 00:32:07]

Done.

+            return false;
+        }
+        synchronized (mSet) {
+            return mSet.remove(obj);
+        }
+    }
+
+    /**
+     * Returns whether current instance contains the object that was previously returned by
+     * {@link add}.
+     */
+    public boolean contains(Object obj) {
+        if (obj == null) {

[Lambros, 2016/05/27 22:26:06]

Maybe remove this test?

[Hzj_jie, 2016/05/28 00:32:06]

Done.

+            return false;
+        }
+        synchronized (mSet) {
+            return mSet.contains(obj);
+        }
+    }
+
+    /**
+     * Returns the total count of runnables attached to current instance.
+     */
+    public int size() {
+        synchronized (mSet) {
+            return mSet.size();
+        }
+    }
+
+    /**
+     * Returns true if there is no runnable attached to current instance.
+     */
+    public boolean isEmpty() {
+        synchronized (mSet) {
+            return mSet.isEmpty();
+        }
+    }
+}