|
|
DescriptionThis change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum.
This change added an Event class, which provides a simpler multiple listeners pattern.
BUG=615277
Committed: https://crrev.com/55898ecf9688df5ce26897362e35667f01eed7c3
Cr-Commit-Position: refs/heads/master@{#396752}
Patch Set 1 #
Total comments: 3
Patch Set 2 : Synchronized mArray.size() calls #Patch Set 3 : Remove WeakReference based event #Patch Set 4 : Use only a Set instead of a Set and a List #
Total comments: 36
Patch Set 5 : Resolve review comments #
Total comments: 37
Patch Set 6 : Resolve review comments #
Messages
Total messages: 33 (13 generated)
Description was changed from ========== Add Event and EventTest BUG= ========== to ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. BUG= ==========
Patchset #1 (id:1) has been deleted
zijiehe@chromium.org changed reviewers: + lambroslambrou@chromium.org
Description was changed from ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. BUG= ========== to ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG= ==========
zijiehe@chromium.org changed reviewers: - lambroslambrou@chromium.org
Patchset #2 (id:40001) has been deleted
zijiehe@chromium.org changed reviewers: + joedow@chromium.org, lambroslambrou@chromium.org
This change is ready to be reviewed.
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = 0; i < mArray.size(); i++) { This is memory ordering safe, in Java, "After we exit a synchronized block, we release the monitor, which has the effect of flushing the cache to main memory, so that writes made by this thread can be visible to other threads." (Refer to wikipedia, https://en.wikipedia.org/wiki/Java_concurrency)
https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = 0; i < mArray.size(); i++) { On 2016/05/23 23:39:14, Hzj_jie wrote: > This is memory ordering safe, in Java, "After we exit a synchronized block, we > release the monitor, which has the effect of flushing the cache to main memory, > so that writes made by this thread can be visible to other threads." (Refer to > wikipedia, https://en.wikipedia.org/wiki/Java_concurrency) I think that quote is misleading, and you do need synchronization in this case. Imagine that ArrayList.add() were implemented something like this: void add(T foo) { int new_size = mSize + 1; mSize = some_random_CPU_register; // freeing up that register ... reallocate buffer, insert foo into it ... // Restore the correct size. mSize = new_size; } This would be a legal, conforming implementation. Nothing in the JMM prevents an optimizing compiler (or JIT compiler, or CPU hardware) from executing this equivalent code. Calling ArrayList.size() at the wrong time, without synchronization, can then return the content of some random CPU register or memory location. See also: http://blogs.grammatech.com/fun-with-concurrency-problems I suggest making a shallow copy of the array, with synchronization. Then iterate over the copied array (no synchronization needed).
https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = 0; i < mArray.size(); i++) { On 2016/05/24 17:56:41, Lambros wrote: > On 2016/05/23 23:39:14, Hzj_jie wrote: > > This is memory ordering safe, in Java, "After we exit a synchronized block, we > > release the monitor, which has the effect of flushing the cache to main > memory, > > so that writes made by this thread can be visible to other threads." (Refer to > > wikipedia, https://en.wikipedia.org/wiki/Java_concurrency) > > I think that quote is misleading, and you do need synchronization in this case. > > Imagine that ArrayList.add() were implemented something like this: > > void add(T foo) { > int new_size = mSize + 1; > > mSize = some_random_CPU_register; // freeing up that register > > ... reallocate buffer, insert foo into it ... > > // Restore the correct size. > mSize = new_size; > } > > This would be a legal, conforming implementation. Nothing in the JMM prevents an > optimizing compiler (or JIT compiler, or CPU hardware) from executing this > equivalent code. > > Calling ArrayList.size() at the wrong time, without synchronization, can then > return the content of some random CPU register or memory location. > > See also: http://blogs.grammatech.com/fun-with-concurrency-problems > > I suggest making a shallow copy of the array, with synchronization. > Then iterate over the copied array (no synchronization needed). I totally agree we would get a random value without synchronized protection in your scenario. But I doubt whether arraylist would be implemented like this. I would implement it like this, void add(T foo) { reallocateIfNeeded(mSize + 1); mArray[mSize++] = foo; } int size() { return mSize; } So if an add function is called concurrently with a size function call, the worst case is, we get an out-of-date value. Indeed in multithreading programming, the out-of-date definition is not so accurate. i.e. we cannot confirm which thread reach the raced condition first. So I have dug out a little bit, and found both implementation of openjdk (http://goo.gl/BdKjk3) and android (https://goo.gl/8HnyIQ), both ensured mSize won't be set to a random value during add operation.
On 2016/05/24 18:24:38, Hzj_jie wrote: > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... > File remoting/android/java/src/org/chromium/chromoting/Event.java (right): > > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... > remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = 0; > i < mArray.size(); i++) { > On 2016/05/24 17:56:41, Lambros wrote: > > On 2016/05/23 23:39:14, Hzj_jie wrote: > > > This is memory ordering safe, in Java, "After we exit a synchronized block, > we > > > release the monitor, which has the effect of flushing the cache to main > > memory, > > > so that writes made by this thread can be visible to other threads." (Refer > to > > > wikipedia, https://en.wikipedia.org/wiki/Java_concurrency) > > > > I think that quote is misleading, and you do need synchronization in this > case. > > > > Imagine that ArrayList.add() were implemented something like this: > > > > void add(T foo) { > > int new_size = mSize + 1; > > > > mSize = some_random_CPU_register; // freeing up that register > > > > ... reallocate buffer, insert foo into it ... > > > > // Restore the correct size. > > mSize = new_size; > > } > > > > This would be a legal, conforming implementation. Nothing in the JMM prevents > an > > optimizing compiler (or JIT compiler, or CPU hardware) from executing this > > equivalent code. > > > > Calling ArrayList.size() at the wrong time, without synchronization, can then > > return the content of some random CPU register or memory location. > > > > See also: http://blogs.grammatech.com/fun-with-concurrency-problems > > > > I suggest making a shallow copy of the array, with synchronization. > > Then iterate over the copied array (no synchronization needed). > > I totally agree we would get a random value without synchronized protection in > your scenario. But I doubt whether arraylist would be implemented like this. I > would implement it like this, > > void add(T foo) { > reallocateIfNeeded(mSize + 1); > mArray[mSize++] = foo; > } > > int size() { > return mSize; > } > > So if an add function is called concurrently with a size function call, the > worst case is, we get an out-of-date value. Indeed in multithreading > programming, the out-of-date definition is not so accurate. i.e. we cannot > confirm which thread reach the raced condition first. > > So I have dug out a little bit, and found both implementation of openjdk > (http://goo.gl/BdKjk3) and android (https://goo.gl/8HnyIQ), both ensured mSize > won't be set to a random value during add operation. Firstly: They might be implemented that way today, but not tomorrow. Secondly: Optimizing compilers, JIT and CPU hardware can transform the source code that you cite, into an equivalent object-code implementation similar to the example I gave. See the article I linked, and the linked paper about "benign data races". They give examples similar to mine, showing how optimizers that move data between memory and registers can break non-synchronized access.
On 2016/05/24 18:37:02, Lambros wrote: > On 2016/05/24 18:24:38, Hzj_jie wrote: > > > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... > > File remoting/android/java/src/org/chromium/chromoting/Event.java (right): > > > > > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... > > remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i = > 0; > > i < mArray.size(); i++) { > > On 2016/05/24 17:56:41, Lambros wrote: > > > On 2016/05/23 23:39:14, Hzj_jie wrote: > > > > This is memory ordering safe, in Java, "After we exit a synchronized > block, > > we > > > > release the monitor, which has the effect of flushing the cache to main > > > memory, > > > > so that writes made by this thread can be visible to other threads." > (Refer > > to > > > > wikipedia, https://en.wikipedia.org/wiki/Java_concurrency) > > > > > > I think that quote is misleading, and you do need synchronization in this > > case. > > > > > > Imagine that ArrayList.add() were implemented something like this: > > > > > > void add(T foo) { > > > int new_size = mSize + 1; > > > > > > mSize = some_random_CPU_register; // freeing up that register > > > > > > ... reallocate buffer, insert foo into it ... > > > > > > // Restore the correct size. > > > mSize = new_size; > > > } > > > > > > This would be a legal, conforming implementation. Nothing in the JMM > prevents > > an > > > optimizing compiler (or JIT compiler, or CPU hardware) from executing this > > > equivalent code. > > > > > > Calling ArrayList.size() at the wrong time, without synchronization, can > then > > > return the content of some random CPU register or memory location. > > > > > > See also: http://blogs.grammatech.com/fun-with-concurrency-problems > > > > > > I suggest making a shallow copy of the array, with synchronization. > > > Then iterate over the copied array (no synchronization needed). > > > > I totally agree we would get a random value without synchronized protection in > > your scenario. But I doubt whether arraylist would be implemented like this. I > > would implement it like this, > > > > void add(T foo) { > > reallocateIfNeeded(mSize + 1); > > mArray[mSize++] = foo; > > } > > > > int size() { > > return mSize; > > } > > > > So if an add function is called concurrently with a size function call, the > > worst case is, we get an out-of-date value. Indeed in multithreading > > programming, the out-of-date definition is not so accurate. i.e. we cannot > > confirm which thread reach the raced condition first. > > > > So I have dug out a little bit, and found both implementation of openjdk > > (http://goo.gl/BdKjk3) and android (https://goo.gl/8HnyIQ), both ensured mSize > > won't be set to a random value during add operation. > > Firstly: They might be implemented that way today, but not tomorrow. > Secondly: Optimizing compilers, JIT and CPU hardware can transform the > source code that you cite, into an equivalent object-code implementation > similar to the example I gave. See the article I linked, and the linked > paper about "benign data races". > They give examples similar to mine, showing how optimizers that move > data between memory and registers can break non-synchronized access. For the second, JIT memory modeling guarantees 1. operations are not reordering, 2. volatile won't be violated, (https://goo.gl/NAvtHk). So most of the synchronization issues in pre c++14 world won't impact java. Considering this is a minor issue, I am pleasure to wrap the size call with a lock to address the first issue you have mentioned.
On 2016/05/24 20:39:56, Hzj_jie wrote: > On 2016/05/24 18:37:02, Lambros wrote: > > On 2016/05/24 18:24:38, Hzj_jie wrote: > > > > > > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... > > > File remoting/android/java/src/org/chromium/chromoting/Event.java (right): > > > > > > > > > https://codereview.chromium.org/1999583002/diff/60001/remoting/android/java/s... > > > remoting/android/java/src/org/chromium/chromoting/Event.java:53: for (int i > = > > 0; > > > i < mArray.size(); i++) { > > > On 2016/05/24 17:56:41, Lambros wrote: > > > > On 2016/05/23 23:39:14, Hzj_jie wrote: > > > > > This is memory ordering safe, in Java, "After we exit a synchronized > > block, > > > we > > > > > release the monitor, which has the effect of flushing the cache to main > > > > memory, > > > > > so that writes made by this thread can be visible to other threads." > > (Refer > > > to > > > > > wikipedia, https://en.wikipedia.org/wiki/Java_concurrency) > > > > > > > > I think that quote is misleading, and you do need synchronization in this > > > case. > > > > > > > > Imagine that ArrayList.add() were implemented something like this: > > > > > > > > void add(T foo) { > > > > int new_size = mSize + 1; > > > > > > > > mSize = some_random_CPU_register; // freeing up that register > > > > > > > > ... reallocate buffer, insert foo into it ... > > > > > > > > // Restore the correct size. > > > > mSize = new_size; > > > > } > > > > > > > > This would be a legal, conforming implementation. Nothing in the JMM > > prevents > > > an > > > > optimizing compiler (or JIT compiler, or CPU hardware) from executing this > > > > equivalent code. > > > > > > > > Calling ArrayList.size() at the wrong time, without synchronization, can > > then > > > > return the content of some random CPU register or memory location. > > > > > > > > See also: http://blogs.grammatech.com/fun-with-concurrency-problems > > > > > > > > I suggest making a shallow copy of the array, with synchronization. > > > > Then iterate over the copied array (no synchronization needed). > > > > > > I totally agree we would get a random value without synchronized protection > in > > > your scenario. But I doubt whether arraylist would be implemented like this. > I > > > would implement it like this, > > > > > > void add(T foo) { > > > reallocateIfNeeded(mSize + 1); > > > mArray[mSize++] = foo; > > > } > > > > > > int size() { > > > return mSize; > > > } > > > > > > So if an add function is called concurrently with a size function call, the > > > worst case is, we get an out-of-date value. Indeed in multithreading > > > programming, the out-of-date definition is not so accurate. i.e. we cannot > > > confirm which thread reach the raced condition first. > > > > > > So I have dug out a little bit, and found both implementation of openjdk > > > (http://goo.gl/BdKjk3) and android (https://goo.gl/8HnyIQ), both ensured > mSize > > > won't be set to a random value during add operation. > > > > Firstly: They might be implemented that way today, but not tomorrow. > > Secondly: Optimizing compilers, JIT and CPU hardware can transform the > > source code that you cite, into an equivalent object-code implementation > > similar to the example I gave. See the article I linked, and the linked > > paper about "benign data races". > > They give examples similar to mine, showing how optimizers that move > > data between memory and registers can break non-synchronized access. > > For the second, JIT memory modeling guarantees 1. operations are not reordering, > 2. volatile won't be violated, (https://goo.gl/NAvtHk). So most of the > synchronization issues in pre c++14 world won't impact java. > Considering this is a minor issue, I am pleasure to wrap the size call with a > lock to address the first issue you have mentioned. Regarding the issues we have talked offline. 1. Is there an Android or Java internal implementation of multiple listener? Unfortunately, I have not found one. Bunch of people have asked this question, one of them http://goo.gl/ZJBMRQ. I am also pretty confusing, it looks like a simple and useful structure, but both Android and Java does not implement it. They may want you to avoid to use multithreading in listener pattern. 2. Why do I use a set of ints and a list of runnables, instead of only a set of runnables? Current implementation is more complex. AFAICT, this can help consumers to write simpler code. i.e. you do not need to store the original runnable instance just for deleting it. Comparing to, ---- Event.ParameterRunnable<Void> r = new Event.ParameterRunnable<>() { @Override public void run(Void nil) { // Do something. } } event.add(r); // ..... event.remove(r); ---- Following logic looks a little bit beautiful (my personal opinion :P). ---- int r = event.add(new Event.ParameterRunnable<Void>() { @Override public void run(Void nil) { // Do something. } }); // ..... event.remove(r); ---- Since we will have consumers of this class everywhere, comparing to a more complex consumer pattern, a little bit more complex provider is more efficient. It's my personal opinion, I do not have a strong preference on both solution. So if you really think it matters, I can surely change to one set solution. 3. Why is there a weak-reference implementation? It was original designed for animators, i.e. we do not need to actively remove the animator callbacks. But soon I found SelfRemovable can actually handle it. I will remove it.
> Regarding the issues we have talked offline. > 1. Is there an Android or Java internal implementation of multiple listener? > Unfortunately, I have not found one. Bunch of people have asked this question, > one of them http://goo.gl/ZJBMRQ. I am also pretty confusing, it looks like a > simple and useful structure, but both Android and Java does not implement it. > They may want you to avoid to use multithreading in listener pattern. Why don't you use one of the collections classes in java.util.concurrent? https://developer.android.com/reference/java/util/concurrent/package-summary.... Maybe ConcurrentHashMap or ConcurrentSkipListSet ? All you would need to do is to wrap this collection inside your own custom class, delegate the add()/remove() operations to the collection, and implement your own raise() method that simply iterates over the collection and calls run() on each entry? This would be much simpler to implement and review, and you almost wouldn't need to write any tests :) Well, OK, you could just have a few basic tests, but you can mostly assume that the util.concurrent classes work as advertised. > > 2. Why do I use a set of ints and a list of runnables, instead of only a set of > runnables? Current implementation is more complex. > AFAICT, this can help consumers to write simpler code. i.e. you do not need to > store the original runnable instance just for deleting it. Comparing to, > ---- > Event.ParameterRunnable<Void> r = new Event.ParameterRunnable<>() { > @Override > public void run(Void nil) { > // Do something. > } > } > event.add(r); > // ..... > event.remove(r); > ---- > Following logic looks a little bit beautiful (my personal opinion :P). > ---- > int r = event.add(new Event.ParameterRunnable<Void>() { > @Override > public void run(Void nil) { > // Do something. > } > }); > // ..... > event.remove(r); I don't agree this is simpler. It only looks that way in your example because your event.add() returns an int. But equally in your first example, you could have made event.add() return the Event.ParameterRunnable, and then your examples would both look the same. I slightly prefer to use object references instead of artificially adding int IDs, but I don't feel too strongly. For example, you might write your own implementation of Event.ParameterRunnable, with a method to remove it from its Event, and you wouldn't need to store any extra int ID.
On 2016/05/25 21:43:48, Lambros wrote: > > Regarding the issues we have talked offline. > > 1. Is there an Android or Java internal implementation of multiple listener? > > Unfortunately, I have not found one. Bunch of people have asked this question, > > one of them http://goo.gl/ZJBMRQ. I am also pretty confusing, it looks like a > > simple and useful structure, but both Android and Java does not implement it. > > They may want you to avoid to use multithreading in listener pattern. > > Why don't you use one of the collections classes in java.util.concurrent? > https://developer.android.com/reference/java/util/concurrent/package-summary.... > > Maybe ConcurrentHashMap or ConcurrentSkipListSet ? > > All you would need to do is to wrap this collection inside your own custom > class, > delegate the add()/remove() operations to the collection, and implement your own > raise() method that simply iterates over the collection and calls run() on each > entry? > > This would be much simpler to implement and review, and you almost wouldn't > need to write any tests :) Well, OK, you could just have a few basic tests, > but you can mostly assume that the util.concurrent classes work as advertised. > > > > > 2. Why do I use a set of ints and a list of runnables, instead of only a set > of > > runnables? Current implementation is more complex. > > AFAICT, this can help consumers to write simpler code. i.e. you do not need to > > store the original runnable instance just for deleting it. Comparing to, > > ---- > > Event.ParameterRunnable<Void> r = new Event.ParameterRunnable<>() { > > @Override > > public void run(Void nil) { > > // Do something. > > } > > } > > event.add(r); > > // ..... > > event.remove(r); > > ---- > > Following logic looks a little bit beautiful (my personal opinion :P). > > ---- > > int r = event.add(new Event.ParameterRunnable<Void>() { > > @Override > > public void run(Void nil) { > > // Do something. > > } > > }); > > // ..... > > event.remove(r); > > I don't agree this is simpler. It only looks that way in your example because > your event.add() returns an int. But equally in your first example, you could > have made event.add() return the Event.ParameterRunnable, and then your > examples would both look the same. > > I slightly prefer to use object references instead of artificially adding > int IDs, but I don't feel too strongly. For example, you might write your > own implementation of Event.ParameterRunnable, with a method to > remove it from its Event, and you wouldn't need to store any extra int > ID. Unfortunately ConcurrentSkipListSet requires the generic type to be java.lang.Comparable, so I am now using only HashSet with an object as lock. I agree the original design is too complex and is not worthy its performance improvement.
On 2016/05/25 23:18:51, Hzj_jie wrote: > On 2016/05/25 21:43:48, Lambros wrote: > > > Regarding the issues we have talked offline. > > > 1. Is there an Android or Java internal implementation of multiple listener? > > > Unfortunately, I have not found one. Bunch of people have asked this > question, > > > one of them http://goo.gl/ZJBMRQ. I am also pretty confusing, it looks like > a > > > simple and useful structure, but both Android and Java does not implement > it. > > > They may want you to avoid to use multithreading in listener pattern. > > > > Why don't you use one of the collections classes in java.util.concurrent? > > > https://developer.android.com/reference/java/util/concurrent/package-summary.... > > > > Maybe ConcurrentHashMap or ConcurrentSkipListSet ? > > > > All you would need to do is to wrap this collection inside your own custom > > class, > > delegate the add()/remove() operations to the collection, and implement your > own > > raise() method that simply iterates over the collection and calls run() on > each > > entry? > > > > This would be much simpler to implement and review, and you almost wouldn't > > need to write any tests :) Well, OK, you could just have a few basic tests, > > but you can mostly assume that the util.concurrent classes work as advertised. > > > > > > > > 2. Why do I use a set of ints and a list of runnables, instead of only a set > > of > > > runnables? Current implementation is more complex. > > > AFAICT, this can help consumers to write simpler code. i.e. you do not need > to > > > store the original runnable instance just for deleting it. Comparing to, > > > ---- > > > Event.ParameterRunnable<Void> r = new Event.ParameterRunnable<>() { > > > @Override > > > public void run(Void nil) { > > > // Do something. > > > } > > > } > > > event.add(r); > > > // ..... > > > event.remove(r); > > > ---- > > > Following logic looks a little bit beautiful (my personal opinion :P). > > > ---- > > > int r = event.add(new Event.ParameterRunnable<Void>() { > > > @Override > > > public void run(Void nil) { > > > // Do something. > > > } > > > }); > > > // ..... > > > event.remove(r); > > > > I don't agree this is simpler. It only looks that way in your example because > > your event.add() returns an int. But equally in your first example, you could > > have made event.add() return the Event.ParameterRunnable, and then your > > examples would both look the same. > > > > I slightly prefer to use object references instead of artificially adding > > int IDs, but I don't feel too strongly. For example, you might write your > > own implementation of Event.ParameterRunnable, with a method to > > remove it from its Event, and you wouldn't need to store any extra int > > ID. > > Unfortunately ConcurrentSkipListSet requires the generic type to be > java.lang.Comparable, so I am now using only HashSet with an object as lock. > I agree the original design is too complex and is not worthy its performance > improvement. This is OK for now. We can always optimize later by using, maybe, ConcurrentHashMap somehow.
Please don't link to internal documents in the CL description. Any links in Chromium should be viewable by everyone. I took a look, and didn't see anything that needs to be private. You could create a new doc from your chromium.org account and copy the content into it. In this case, it's just a refactoring plan, so I think it would be OK to keep it within our team and not publish the link. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:33: * An event provider version of {@link Event} implementation, provides {@link raise} function to 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. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:50: Preconditions.notNull(set); Don't need this Preconditions. It's obvious that eventSet() can't be null, and you de-reference |set| just two lines below. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:51: int r = 0; Avoid single-letter names (for-loop variables and temporaries are OK). Use 'result' or 'count' instead. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:63: private Set<ParameterRunnable<ParamType>> eventSet() { If you use toArray() instead of clone(), you don't need this method or @SuppressWarnings. And toArray() is probably more efficient. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:76: private final ParameterCallback<Boolean, ParamType> mRef; Don't abbreviate names (in general; sometimes it's OK). Maybe mCallback? https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:78: // This lock is used to make sure mId is correctly set before remove in run function. i.e. Blank line before comment. Also, mId doesn't exist, so I don't understand the purpose of the lock. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:85: Preconditions.notNull(owner); 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. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:91: mEvent = mOwner.add(this); 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()? https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:99: Preconditions.isTrue(mOwner.remove(mEvent)); 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. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:105: // instance, but all the logic is in new function. I don't think this should be called create() - it doesn't return any object. Maybe addCallbackToEvent()? https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:114: protected final Object mLock; Since mSet is final, you don't need a separate mLock object. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:140: * {@link SelfRemovableParameterRunnable} will remove itself from {@link Event} instance, this SelfRemovableParameterRunnable is a private implementation detail, so don't expose it in the JavaDoc. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:141: * function returns a boolean value to indicate the success or not of the operation. 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. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:152: * Removes an object represents an event and returned by {@link add} function from this s/represents/representing Or maybe rephrase: Removes an object that was previously returned by {@link add}. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:175: * Returns true if there are at least one runnable attached to current instance. s/are/is https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:176: */ 'attached' is maybe a little bit strange. Maybe hasAttachedRunnables() ? I would be tempted to invert it and call it isEmpty(). https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Preconditions.java (right): https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Preconditions.java:16: // Checks whether input |v| is true, and returns its value. Throws s/v/value Why bother returning the value if it's always true?
On 2016/05/26 19:32:02, Lambros wrote: > On 2016/05/25 23:18:51, Hzj_jie wrote: > > On 2016/05/25 21:43:48, Lambros wrote: > > > > Regarding the issues we have talked offline. > > > > 1. Is there an Android or Java internal implementation of multiple > listener? > > > > Unfortunately, I have not found one. Bunch of people have asked this > > question, > > > > one of them http://goo.gl/ZJBMRQ. I am also pretty confusing, it looks > like > > a > > > > simple and useful structure, but both Android and Java does not implement > > it. > > > > They may want you to avoid to use multithreading in listener pattern. > > > > > > Why don't you use one of the collections classes in java.util.concurrent? > > > > > > https://developer.android.com/reference/java/util/concurrent/package-summary.... > > > > > > Maybe ConcurrentHashMap or ConcurrentSkipListSet ? > > > > > > All you would need to do is to wrap this collection inside your own custom > > > class, > > > delegate the add()/remove() operations to the collection, and implement your > > own > > > raise() method that simply iterates over the collection and calls run() on > > each > > > entry? > > > > > > This would be much simpler to implement and review, and you almost wouldn't > > > need to write any tests :) Well, OK, you could just have a few basic tests, > > > but you can mostly assume that the util.concurrent classes work as > advertised. > > > > > > > > > > > 2. Why do I use a set of ints and a list of runnables, instead of only a > set > > > of > > > > runnables? Current implementation is more complex. > > > > AFAICT, this can help consumers to write simpler code. i.e. you do not > need > > to > > > > store the original runnable instance just for deleting it. Comparing to, > > > > ---- > > > > Event.ParameterRunnable<Void> r = new Event.ParameterRunnable<>() { > > > > @Override > > > > public void run(Void nil) { > > > > // Do something. > > > > } > > > > } > > > > event.add(r); > > > > // ..... > > > > event.remove(r); > > > > ---- > > > > Following logic looks a little bit beautiful (my personal opinion :P). > > > > ---- > > > > int r = event.add(new Event.ParameterRunnable<Void>() { > > > > @Override > > > > public void run(Void nil) { > > > > // Do something. > > > > } > > > > }); > > > > // ..... > > > > event.remove(r); > > > > > > I don't agree this is simpler. It only looks that way in your example > because > > > your event.add() returns an int. But equally in your first example, you > could > > > have made event.add() return the Event.ParameterRunnable, and then your > > > examples would both look the same. > > > > > > I slightly prefer to use object references instead of artificially adding > > > int IDs, but I don't feel too strongly. For example, you might write your > > > own implementation of Event.ParameterRunnable, with a method to > > > remove it from its Event, and you wouldn't need to store any extra int > > > ID. > > > > Unfortunately ConcurrentSkipListSet requires the generic type to be > > java.lang.Comparable, so I am now using only HashSet with an object as lock. > > I agree the original design is too complex and is not worthy its performance > > improvement. > > This is OK for now. We can always optimize later by using, maybe, > ConcurrentHashMap somehow. I will try.
Also, this work is substantial enough that you should probably file a bug and include the BUG= number in the CL description.
Description was changed from ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG= ========== to ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG=615277 ==========
https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:33: * An event provider version of {@link Event} implementation, provides {@link raise} function to On 2016/05/27 00:38:51, Lambros wrote: > 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. 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. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:50: Preconditions.notNull(set); On 2016/05/27 00:38:52, Lambros wrote: > Don't need this Preconditions. It's obvious that eventSet() can't be null, and > you de-reference |set| just two lines below. Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:51: int r = 0; On 2016/05/27 00:38:51, Lambros wrote: > Avoid single-letter names (for-loop variables and temporaries are OK). > Use 'result' or 'count' instead. Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:63: private Set<ParameterRunnable<ParamType>> eventSet() { On 2016/05/27 00:38:52, Lambros wrote: > If you use toArray() instead of clone(), you don't need this method or > @SuppressWarnings. > And toArray() is probably more efficient. Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:76: private final ParameterCallback<Boolean, ParamType> mRef; On 2016/05/27 00:38:52, Lambros wrote: > Don't abbreviate names (in general; sometimes it's OK). Maybe mCallback? Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:78: // This lock is used to make sure mId is correctly set before remove in run function. i.e. On 2016/05/27 00:38:51, Lambros wrote: > Blank line before comment. > Also, mId doesn't exist, so I don't understand the purpose of the lock. 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} https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:85: Preconditions.notNull(owner); On 2016/05/27 00:38:51, Lambros wrote: > 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. > Yes, updated. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:91: mEvent = mOwner.add(this); On 2016/05/27 00:38:51, Lambros wrote: > 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()? 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. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:99: Preconditions.isTrue(mOwner.remove(mEvent)); On 2016/05/27 00:38:52, Lambros wrote: > 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. > 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. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:105: // instance, but all the logic is in new function. On 2016/05/27 00:38:51, Lambros wrote: > I don't think this should be called create() - it doesn't return any object. > Maybe addCallbackToEvent()? Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:114: protected final Object mLock; On 2016/05/27 00:38:51, Lambros wrote: > Since mSet is final, you don't need a separate mLock object. Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:140: * {@link SelfRemovableParameterRunnable} will remove itself from {@link Event} instance, this On 2016/05/27 00:38:51, Lambros wrote: > SelfRemovableParameterRunnable is a private implementation detail, so don't > expose it in the JavaDoc. Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:141: * function returns a boolean value to indicate the success or not of the operation. On 2016/05/27 00:38:51, Lambros wrote: > 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. Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:152: * Removes an object represents an event and returned by {@link add} function from this On 2016/05/27 00:38:52, Lambros wrote: > s/represents/representing > > Or maybe rephrase: > Removes an object that was previously returned by {@link add}. Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:175: * Returns true if there are at least one runnable attached to current instance. On 2016/05/27 00:38:51, Lambros wrote: > s/are/is Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:176: */ On 2016/05/27 00:38:52, Lambros wrote: > 'attached' is maybe a little bit strange. Maybe hasAttachedRunnables() ? > I would be tempted to invert it and call it isEmpty(). > Done. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Preconditions.java (right): https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Preconditions.java:16: // Checks whether input |v| is true, and returns its value. Throws On 2016/05/27 00:38:52, Lambros wrote: > s/v/value > > Why bother returning the value if it's always true? I think it's more helpful in the following scenario, bool do_something_else(...) { if (...) { // parameters are valid combinations. return true; return false; } bool do_something(...) { if (...) { return false; } // ... do something // the parameters we have sent to do_something_else should always valid. return Preconditions.isTrue(do_something_else(...)); }
lgtm when nits are addressed. Please read my comments. I've made some suggestions, but I'm happy to leave them up to your judgement. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:99: Preconditions.isTrue(mOwner.remove(mEvent)); On 2016/05/27 02:55:19, Hzj_jie wrote: > On 2016/05/27 00:38:52, Lambros wrote: > > 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. > > > > 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. 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. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:11: * O(log(n)) time complex, and a {@link raise} function in Executor inheritance to execute all s/complex/complexity/ s/Executor inheritance/the derived class {@link Event.Executor}/ https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:12: * queued object. s/object/objects/ https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:14: * @param <ParamType> The parameter used in {@link ParameterRunnable} callback. 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) https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:18: * A runnable with parameter. Elsewhere, we've formatted single-line JavaDoc as /** Foo bar. */ Do the same here for consistency? https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:36: public void clear() { Need JavaDoc for public methods (linter will complain, probably). https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:63: @SuppressWarnings("unchecked") 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. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:91: mEvent = mOwner.add(this); 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) https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:131: * Adds a self removable {@link ParameterRunnable} object into current instance, the runnable Grammar nit: This is 2 sentences, so: Either s/,/;/ Or s/, the/. The/ https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:136: // SelfRemovableParameterRunner is self-contained, i.e. consumers do not need to have a Blank line before comment. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:146: if (obj == null) { Maybe remove this test? https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:159: if (obj == null) { Maybe remove this test? https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... File remoting/android/javatests/src/org/chromium/chromoting/Counter.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Counter.java:10: * thread-safe, for a thread-safe implementation, use Grammar nit: Replace first , with ; https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Counter.java:13: public class Counter { Put this in a 'util' sub-package, and maybe call it MutableInteger. See https://www.chromium.org/developers/testing/android-tests under "Utility test classes". https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Counter.java:14: private int mV; 'mValue' preferred (and 'value' for parameters). It's OK to make this field public, if you prefer. If you do this, the field should be named 'value' (no 'm' prefix, begin with lower-case letter). https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... File remoting/android/javatests/src/org/chromium/chromoting/EventTest.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/EventTest.java:38: assertTrue(false); (Here and elsewhere) Use fail() instead of assertTrue(false). You can optionally pass in a String message. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/EventTest.java:57: private final Pointer<Boolean> mError; Would a MutableBoolean class work better here? (and then Pointer is no longer used anywhere) https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/EventTest.java:73: public void run(Void nil) { Maybe just run(Void) ? https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... File remoting/android/javatests/src/org/chromium/chromoting/Pointer.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Pointer.java:8: * A container of a reference to pass data into or out from a closure. This class is usually useful This should go in a 'util' subpackage. Maybe call it MutableReference?
Thank you for your review, I have resolved most of the issues you have mentioned. Since the code is not actively used in production, I will submit it first. Feel free to drop me a message for any potential issues. https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/120001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:99: Preconditions.isTrue(mOwner.remove(mEvent)); On 2016/05/27 22:26:06, Lambros wrote: > On 2016/05/27 02:55:19, Hzj_jie wrote: > > On 2016/05/27 00:38:52, Lambros wrote: > > > 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. > > > > > > > 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. > > 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. 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. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:11: * O(log(n)) time complex, and a {@link raise} function in Executor inheritance to execute all On 2016/05/27 22:26:06, Lambros wrote: > s/complex/complexity/ > s/Executor inheritance/the derived class {@link Event.Executor}/ Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:12: * queued object. On 2016/05/27 22:26:06, Lambros wrote: > s/object/objects/ Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:14: * @param <ParamType> The parameter used in {@link ParameterRunnable} callback. On 2016/05/27 22:26:06, Lambros wrote: > 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) Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:18: * A runnable with parameter. On 2016/05/27 22:26:06, Lambros wrote: > Elsewhere, we've formatted single-line JavaDoc as > /** Foo bar. */ > Do the same here for consistency? I did not see a coding style for this, but yes, we should keep consistency. Updated. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:36: public void clear() { On 2016/05/27 22:26:06, Lambros wrote: > Need JavaDoc for public methods (linter will complain, probably). Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:63: @SuppressWarnings("unchecked") On 2016/05/27 22:26:06, Lambros wrote: > 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. 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? https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:91: mEvent = mOwner.add(this); On 2016/05/27 22:26:06, Lambros wrote: > 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) 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. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:131: * Adds a self removable {@link ParameterRunnable} object into current instance, the runnable On 2016/05/27 22:26:06, Lambros wrote: > Grammar nit: > This is 2 sentences, so: > Either s/,/;/ > Or s/, the/. The/ Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:136: // SelfRemovableParameterRunner is self-contained, i.e. consumers do not need to have a On 2016/05/27 22:26:07, Lambros wrote: > Blank line before comment. Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:146: if (obj == null) { On 2016/05/27 22:26:06, Lambros wrote: > Maybe remove this test? Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:159: if (obj == null) { On 2016/05/27 22:26:06, Lambros wrote: > Maybe remove this test? Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... File remoting/android/javatests/src/org/chromium/chromoting/Counter.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Counter.java:10: * thread-safe, for a thread-safe implementation, use On 2016/05/27 22:26:07, Lambros wrote: > Grammar nit: Replace first , with ; Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Counter.java:13: public class Counter { On 2016/05/27 22:26:07, Lambros wrote: > Put this in a 'util' sub-package, and maybe call it MutableInteger. > See https://www.chromium.org/developers/testing/android-tests > under "Utility test classes". Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Counter.java:14: private int mV; On 2016/05/27 22:26:07, Lambros wrote: > 'mValue' preferred (and 'value' for parameters). > It's OK to make this field public, if you prefer. > If you do this, the field should be named 'value' (no 'm' prefix, begin with > lower-case letter). Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... File remoting/android/javatests/src/org/chromium/chromoting/EventTest.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/EventTest.java:38: assertTrue(false); On 2016/05/27 22:26:07, Lambros wrote: > (Here and elsewhere) Use fail() instead of assertTrue(false). > You can optionally pass in a String message. Done. https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/EventTest.java:57: private final Pointer<Boolean> mError; On 2016/05/27 22:26:07, Lambros wrote: > Would a MutableBoolean class work better here? > (and then Pointer is no longer used anywhere) I would prefer to remove Counter / MutableInteger, since Pointer is more common, we do not need to write a new class for certain usage. i.e. MutableLong / MutableFloat ... https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/EventTest.java:73: public void run(Void nil) { On 2016/05/27 22:26:07, Lambros wrote: > Maybe just run(Void) ? I do not think Java support this syntax. :) https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... File remoting/android/javatests/src/org/chromium/chromoting/Pointer.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/javat... remoting/android/javatests/src/org/chromium/chromoting/Pointer.java:8: * A container of a reference to pass data into or out from a closure. This class is usually useful On 2016/05/27 22:26:07, Lambros wrote: > This should go in a 'util' subpackage. Maybe call it MutableReference? Done.
The CQ bit was checked by zijiehe@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from lambroslambrou@chromium.org Link to the patchset: https://codereview.chromium.org/1999583002/#ps160001 (title: "Resolve review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1999583002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1999583002/160001
Message was sent while issue was closed.
Description was changed from ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG=615277 ========== to ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG=615277 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG=615277 ========== to ========== This change is part of Remote Desktop Android Client Refactor work. A Google internal design doc can be found at http://shortn/_viNREInZum. This change added an Event class, which provides a simpler multiple listeners pattern. BUG=615277 Committed: https://crrev.com/55898ecf9688df5ce26897362e35667f01eed7c3 Cr-Commit-Position: refs/heads/master@{#396752} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/55898ecf9688df5ce26897362e35667f01eed7c3 Cr-Commit-Position: refs/heads/master@{#396752}
Message was sent while issue was closed.
https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... File remoting/android/java/src/org/chromium/chromoting/Event.java (right): https://codereview.chromium.org/1999583002/diff/140001/remoting/android/java/... remoting/android/java/src/org/chromium/chromoting/Event.java:99: if (!mCallback.run(p)) { 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? |