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;
+import java.util.Set;
+
+/**
+ * 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
+ * queued object.
+ *
+ * @param <ParamType> The parameter used in {@link ParameterRunnable} callback.
+ */
+public class Event<ParamType> {
+    /**
+     * A runnable with parameter.
+     */
+    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

[Lambros, 2016/05/27 00:38:51]

Why subclass, instead of adding raise() to Event?

Also, it is strange that the inner class Event.Executor "is an" Event.
Usually, inner classes don't subclass the outer class.
A better name would be something like "RaisableEvent", as a
top-level class. But I don't see why this is useful.

[Hzj_jie, 2016/05/27 02:55:20]

This pattern can avoid any potential misusage of an event. Since an event
provider needs to public its event to event consumers, but consumers should only
be able to add and remove a callback from event queue instead of raise or clear
an event.

I am OK to move it into a top-level class, or use an interface to separate the
implementation. But these two pattern will eventually make it complex.
We won't have other event implementations, so an interface is simply increase
the code size.
Moving to a top-level class means you need to open two files to edit the logic.

+     * execute appended {@link ParameterRunnable}.
+     */
+    public static final class Executor<ParamType> extends Event<ParamType> {
+        public void clear() {
+            synchronized (mLock) {
+                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) {
+            Set<ParameterRunnable<ParamType>> set = eventSet();
+            Preconditions.notNull(set);

[Lambros, 2016/05/27 00:38:52]

Don't need this Preconditions. It's obvious that eventSet() can't be null, and
you de-reference |set| just two lines below.

[Hzj_jie, 2016/05/27 02:55:20]

Done.

+            int r = 0;

[Lambros, 2016/05/27 00:38:51]

Avoid single-letter names (for-loop variables and temporaries are OK).
Use 'result' or 'count' instead.

[Hzj_jie, 2016/05/27 02:55:19]

Done.

+            for (ParameterRunnable<ParamType> e : set) {
+                e.run(parameter);
+                r++;
+            }
+            return r;
+        }
+
+        /**
+         * Returns a shadow copy of {@link mSet} as a {@link Set}.
+         */
+        @SuppressWarnings("unchecked")
+        private Set<ParameterRunnable<ParamType>> eventSet() {

[Lambros, 2016/05/27 00:38:52]

If you use toArray() instead of clone(), you don't need this method or
@SuppressWarnings.
And toArray() is probably more efficient.

[Hzj_jie, 2016/05/27 02:55:19]

Done.

+            synchronized (mLock) {
+                return (Set<ParameterRunnable<ParamType>>) mSet.clone();
+            }
+        }
+    }
+
+    /**
+     * 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> mRef;

[Lambros, 2016/05/27 00:38:52]

Don't abbreviate names (in general; sometimes it's OK). Maybe mCallback?

[Hzj_jie, 2016/05/27 02:55:19]

Done.

+        private final Event<ParamType> mOwner;
+        // This lock is used to make sure mId is correctly set before remove in run function. i.e.

[Lambros, 2016/05/27 00:38:51]

Blank line before comment.
Also, mId doesn't exist, so I don't understand the purpose of the lock.

[Hzj_jie, 2016/05/27 02:55:20]

If the logic runs in following order, it will fail without this lock.
1. mOwner.add(this) to a temp object
2. mOwner.raise(...) -> mOwner.remove(mEvent)
3. mEvent = {temp object in step 1}

+        // mOwner.add and assigment of mId need to be atomic.
+        private final Object mLock;
+        private final Object mEvent;
+
+        private SelfRemovableParameterRunnable(Event<ParamType> owner,
+                                               ParameterCallback<Boolean, ParamType> callback) {
+            Preconditions.notNull(owner);

[Lambros, 2016/05/27 00:38:51]

This is OK for now, but I'd generally avoid over-using Preconditions.
For example, you call mOwner.add(), so a Precondition that mOwner is non-null
seems to add no value.

[Hzj_jie, 2016/05/27 02:55:19]

Yes, updated.

+            Preconditions.notNull(callback);
+            mRef = callback;
+            mOwner = owner;
+            mLock = new Object();
+            synchronized (mLock) {
+                mEvent = mOwner.add(this);

[Lambros, 2016/05/27 00:38:51]

I'm not sure, but this might be too much work to do in the ctor?
Leaking the 'this' object outside the ctor potentially breaks the
thread-safe guarantees about 'final' members (and risks other
problems with incompletely-constructed objects). I think it's
probably OK in this case. You might find that FindBugs
complains about it.
Maybe provide a separate method, attachToEvent()?

[Hzj_jie, 2016/05/27 02:55:20]

If we move some logic in to a new function, we will not be able to set mEvent as
final, so a new function seems not valuable to me.

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

[Lambros, 2016/05/27 00:38:52]

I don't think this is safe if two threads call Event.Executor.raise() at the
same time.
Event.raise():
1. Acquires the lock,
2. Copies the Runnables,
3. Releases the lock,
4. Runs all the copied Runnables.

Between 3 and 4, another thread can sneak in and copy the exact same set of
Runnables:
Thread A does 1,2,3
Thread B does 1,2,3
Thread A does 4
Thread B does 4
Now threads A and B will try to remove the same object twice.

I think this is safer without the Preconditions.

[Hzj_jie, 2016/05/27 02:55:19]

Done, but this class is designed to be single reader and multiple writers. i.e.
only one thread (UI thread in our scenario) can call the raise function. And
it's also the reason I add raise function in a separated subclass.
If we would like to make this class multiple reader safe (I do not think we have
such scenario, since Android has only one UI thread.), I would prefer to add a
contains function for Event, and make sure one callback will never be called
again after it returns false.

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

Even if Event.Executor.raise() is always called from the same thread,
you still have the same race-condition if Event.remove() or
Event.clear() are called on a different thread.
I think it's OK without the Preconditions check.

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

Oh, yes, I forgot Event.Executor.clear. I will remove the Preconditions check,
but keep current solution. It would make the state a little bit more stable,
i.e. if a SelfRemovableParameterRunnable returns false, it will never be called
again.
Also added a line of comment to explain the reason.

+                }
+            }
+        }
+
+        // This class is self-contained, i.e. consumers do not need to have a reference of this
+        // instance, but all the logic is in new function.

[Lambros, 2016/05/27 00:38:51]

I don't think this should be called create() - it doesn't return any object.
Maybe addCallbackToEvent()?

[Hzj_jie, 2016/05/27 02:55:20]

Done.

+        public static final <ParamType> void create(
+                Event<ParamType> owner,
+                ParameterCallback<Boolean, ParamType> callback) {
+            new SelfRemovableParameterRunnable<ParamType>(owner, callback);
+        }
+    }
+
+    protected final HashSet<ParameterRunnable<ParamType>> mSet;
+    protected final Object mLock;

[Lambros, 2016/05/27 00:38:51]

Since mSet is final, you don't need a separate mLock object.

[Hzj_jie, 2016/05/27 02:55:19]

Done.

+
+    public Event() {
+        mLock = new Object();
+        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 (mLock) {
+            if (mSet.add(runnable)) {
+                return runnable;
+            }
+        }
+        return null;
+    }
+
+    /**
+     * Adds a self removable {@link ParameterRunnable} object into current instance, since
+     * {@link SelfRemovableParameterRunnable} will remove itself from {@link Event} instance, this

[Lambros, 2016/05/27 00:38:51]

SelfRemovableParameterRunnable is a private implementation detail, so don't
expose it in the JavaDoc.

[Hzj_jie, 2016/05/27 02:55:19]

Done.

+     * function returns a boolean value to indicate the success or not of the operation.

[Lambros, 2016/05/27 00:38:51]

The boolean return is not needed (it's always true for non-null callback).
Just return void and require that |callback| is not null. Can check this with
Preconditions if you think it's helpful.

[Hzj_jie, 2016/05/27 02:55:19]

Done.

+     */
+    public boolean addSelfRemovable(ParameterCallback<Boolean, ParamType> callback) {
+        if (callback == null) {
+            return false;
+        }
+        SelfRemovableParameterRunnable.<ParamType>create(this, callback);
+        return true;
+    }
+
+    /**
+     * Removes an object represents an event and returned by {@link add} function from this

[Lambros, 2016/05/27 00:38:52]

s/represents/representing

Or maybe rephrase:
Removes an object that was previously returned by {@link add}.

[Hzj_jie, 2016/05/27 02:55:20]

Done.

+     * instance. 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) {
+            return false;
+        }
+        synchronized (mLock) {
+            return mSet.remove(obj);
+        }
+    }
+
+    /**
+     * Returns the total count of runnables attached to current instance.
+     */
+    public int size() {
+        synchronized (mLock) {
+            return mSet.size();
+        }
+    }
+
+    /**
+     * Returns true if there are at least one runnable attached to current instance.

[Lambros, 2016/05/27 00:38:51]

s/are/is

[Hzj_jie, 2016/05/27 02:55:20]

Done.

+     */

[Lambros, 2016/05/27 00:38:52]

'attached' is maybe a little bit strange. Maybe hasAttachedRunnables() ?
I would be tempted to invert it and call it isEmpty().

[Hzj_jie, 2016/05/27 02:55:20]

Done.

+    public boolean attached() {
+        synchronized (mLock) {
+            return !mSet.isEmpty();
+        }
+    }
+}