matthiasblaesing commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443664526
##########
enterprise/javaee.wildfly/src/org/netbeans/modules/javaee/wildfly/ide/ui/AddServerLocationPanel.java:
##########
@@ -44,25 +42,15 @@ public class AddServerLocationPanel implements
WizardDescriptor.FinishablePanel,
private AddServerLocationVisualPanel component;
private WizardDescriptor wizard;
- private final transient Set listeners = ConcurrentHashMap.newKeySet(2);
+ private final transient Set<ChangeListener> listeners =
ConcurrentHashMap.newKeySet(2);
public AddServerLocationPanel(WildflyInstantiatingIterator
instantiatingIterator) {
this.instantiatingIterator = instantiatingIterator;
}
@Override
public void stateChanged(ChangeEvent ev) {
- fireChangeEvent(ev);
- }
-
- private void fireChangeEvent(ChangeEvent ev) {
- Iterator it;
- synchronized (listeners) {
- it = new HashSet(listeners).iterator();
- }
- while (it.hasNext()) {
- ((ChangeListener) it.next()).stateChanged(ev);
- }
+ listeners.iterator().forEachRemaining(l -> l.stateChanged(ev));
Review Comment:
I updated the PR in place. This indeed might(!) have changed behavior, if a
listener modified the listener list while being invoked.
For the safeness:
> The iterator won't protect from concurrent access, the javadoc of CHM says
"However, iterators are designed to be used by only one thread at a time".
The iterator is not accessed concurrently here. `#iterator` creates a new
iterator each time it is invoked and thus the iteration is safe here.
The problem is, that the iterator might(!) reflect changes to the listeners
while iterating and so yes we need a copy to be safe.
The change you propose however would not be correct - while
`Collections#synchronizedSet` explicitly states that the objects monitor is
used for synchronization, the same is not true for `ConcurrentHashMap`. This
demonstrates it:
```java
import java.util.Collections;
import java.util.HashSet;
import java.util.Set;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.ConcurrentHashMap;
public class TestX {
public static void main(String[] args) throws Exception {
CountDownLatch block1 = new CountDownLatch(1);
CountDownLatch block2 = new CountDownLatch(1);
Set<String> concurrentSet = ConcurrentHashMap.<String>newKeySet();
// Set<String> concurrentSet = Collections.synchronizedSet(new
HashSet<>());
Thread t1 = new Thread() {
@Override
public void run() {
synchronized (concurrentSet) {
block1.countDown();
try {
block2.await();
System.out.println(concurrentSet);
} catch (InterruptedException ex) {
System.out.println("Interrupted");
}
}
}
};
Thread t2 = new Thread() {
@Override
public void run() {
try {
block1.await();
concurrentSet.add("Test");
block2.countDown();
} catch (InterruptedException ex) {
System.out.println("Interrupted");
}
}
};
t1.start();
t2.start();
}
}
```
The `t2` waits for `t1` to enter the block synchronized on `concurrentSet`
(the count down latch `block1`). Once `t1` has entered the block, `t2` is
allowed to continue execution (`block1` is cleared). `t1` now waits for `t2` to
try to add the string "Test" to the set by blocking on the count down latch
`block2`. After the string was added `t1` is allowed to continue and prints the
result.
If `ConcurrentHashMap` would use the objects monitor, the execution would
dead-lock `t1` holds the monitor the entire time and `t2` would need to acquire
it for the `concurrentSet.add` operation to succeed. That can be observed by
replacing hte `ConcurrentHashMap` with a `Collections.synchronizedSet(new
HashSet<>())`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
