You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@netbeans.apache.org by "matthiasblaesing (via GitHub)" <gi...@apache.org> on 2023/12/21 19:46:01 UTC
[PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
matthiasblaesing opened a new pull request, #6872:
URL: https://github.com/apache/netbeans/pull/6872
(no comment)
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "mbien (via GitHub)" <gi...@apache.org>.
mbien commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443473341
##########
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:
you sure this is safe? I think what the original code attempted to do is to shapshot a copy of the listeners, and then iterate over that copy, probably anticipating that the listeners set can change at any point in time.
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".
alternative proposal in the spirit of the original:
```java
List<ChangeListener> copy;
synchronized (listeners) {
copy = new ArrayList<>(listeners);
}
copy.forEach(l -> l.stateChanged(ev));
```
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443688082
##########
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 agree with all of this. This was not the point of my fix/simplification. The synchronization that was in the original code could not work. I fixed that. End of story.
What makes me a bit icky is, that we have a massive regression in performance of the IDE because to few people really looked at the fall-out of the multi-file launcher code changes and here, where a fraction of people see the code we are discussing performance? Sorry, I won't make anymore changes to this. If anyone wants to veto this, fine, the that person should at least fix the core problem addressed here.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443690927
##########
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:
Agree entirely! Only caught my eye because of your comment above. However, performance isn't the relevant bit - having things changing or firing listeners off of the EDT is usually a big flashing warning sign.
As for the multi-file regressions, personally I'm in favour of reverting and rethinking after freeze, but waiting to see where that goes ...
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443681373
##########
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:
In general for listeners, surely copying on add/remove is better than copying on fire anyway - eg. `CopyOnWriteArraySet`? Iteration not being randomly ordered is also a good thing here. And why are there concurrency concerns with Swing listeners here in the first place?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#issuecomment-1866841615
@tonihele could you please have a look?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing merged PR #6872:
URL: https://github.com/apache/netbeans/pull/6872
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "tonihele (via GitHub)" <gi...@apache.org>.
tonihele commented on PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#issuecomment-1866887654
> @tonihele could you please have a look?
Looks good to me! Nice work!
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443768840
##########
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:
Behavior is now back to the original "Make a copy of the listener list and then fire"-methology, the code is more compact and there is no useless synchronization. I intent to merge tomorrow.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#issuecomment-1875767399
If noone objects, I plan to merge this next weekend.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "asbachb (via GitHub)" <gi...@apache.org>.
asbachb commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1434648957
##########
enterprise/javaee.wildfly/src/org/netbeans/modules/javaee/wildfly/ide/WildflyJ2eePlatformFactory.java:
##########
@@ -334,21 +334,13 @@ String getDefaultJpaProvider() {
}
private boolean containsJaxWsLibraries() {
- File[] jaxWsAPILib = new File(properties.getModulePath("org/jboss/ws/api/main")).listFiles(new FilenameFilter() {
- @Override
- public boolean accept(File dir, String name) {
- return name.startsWith("jbossws-api") && name.endsWith("jar");
- }
- }); // NOI18N
+ File[] jaxWsAPILib = new File(properties.getModulePath("org/jboss/ws/api/main")) // NOI18N
+ .listFiles((File dir, String name) -> name.startsWith("jbossws-api") && name.endsWith("jar")); // NOI18N
if (jaxWsAPILib != null && jaxWsAPILib.length == 1 && jaxWsAPILib[0].exists()) {
Review Comment:
Is there a possibility that `listFiles` returns a non existent file? So is the check for existence necessary?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "matthiasblaesing (via GitHub)" <gi...@apache.org>.
matthiasblaesing commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1435346496
##########
enterprise/javaee.wildfly/src/org/netbeans/modules/javaee/wildfly/ide/WildflyJ2eePlatformFactory.java:
##########
@@ -334,21 +334,13 @@ String getDefaultJpaProvider() {
}
private boolean containsJaxWsLibraries() {
- File[] jaxWsAPILib = new File(properties.getModulePath("org/jboss/ws/api/main")).listFiles(new FilenameFilter() {
- @Override
- public boolean accept(File dir, String name) {
- return name.startsWith("jbossws-api") && name.endsWith("jar");
- }
- }); // NOI18N
+ File[] jaxWsAPILib = new File(properties.getModulePath("org/jboss/ws/api/main")) // NOI18N
+ .listFiles((File dir, String name) -> name.startsWith("jbossws-api") && name.endsWith("jar")); // NOI18N
if (jaxWsAPILib != null && jaxWsAPILib.length == 1 && jaxWsAPILib[0].exists()) {
Review Comment:
My reading is, that the null check is required, but the existence check is not necessary. However I don't expect this code to be a hot path, so it should be harmless.
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443681373
##########
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:
In general for listeners, surely copying on add/remove is better than copying on fire anyway - eg. `CopyOnWriteArraySet`?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists
Re: [PR] Wildfly Integration further cleanup after dca223ab08b6d96afc81a9a03c4bf97bf238a215 [netbeans]
Posted by "neilcsmith-net (via GitHub)" <gi...@apache.org>.
neilcsmith-net commented on code in PR #6872:
URL: https://github.com/apache/netbeans/pull/6872#discussion_r1443681373
##########
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:
In general for listeners, surely copying on add/remove is better than copying on fire anyway - eg. `CopyOnWriteArraySet`? Or even https://github.com/apache/netbeans/blob/master/platform/openide.util/src/org/openide/util/ChangeSupport.java Iteration not being randomly ordered is also a good thing here. And why are there concurrency concerns with Swing listeners here in the first place?
--
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: notifications-unsubscribe@netbeans.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@netbeans.apache.org
For additional commands, e-mail: notifications-help@netbeans.apache.org
For further information about the NetBeans mailing lists, visit:
https://cwiki.apache.org/confluence/display/NETBEANS/Mailing+lists