matthiasblaesing commented on code in PR #6596:
URL: https://github.com/apache/netbeans/pull/6596#discussion_r1367782801
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -97,7 +99,7 @@ public Props(Map<String, String> map) {
if (map == null) {
throw new NullPointerException("Source Map shall not be
null.");
}
- this.delegate = map;
+ this.delegate = new ConcurrentHashMap<>(map);
Review Comment:
I think this is a good change, as the documentation implies a new map is
constructed, but is this a safe assumption? I mean that before this change, a
caller could modify the Props through the map provided here.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -71,17 +83,17 @@ public final class GlassfishInstanceProvider implements
ServerInstanceProvider,
public static final String JAKARTAEE10_DEPLOYER_FRAGMENT =
"deployer:gfv700ee10";
public static final String EE6WC_DEPLOYER_FRAGMENT = "deployer:gfv3ee6wc";
// NOI18N
public static final String PRELUDE_DEPLOYER_FRAGMENT = "deployer:gfv3"; //
NOI18N
- private static String EE6_INSTANCES_PATH = "/GlassFishEE6/Instances"; //
NOI18N
- private static String EE7_INSTANCES_PATH = "/GlassFishEE7/Instances"; //
NOI18N
- private static String EE8_INSTANCES_PATH = "/GlassFishEE8/Instances"; //
NOI18N
- private static String JAKARTAEE8_INSTANCES_PATH =
"/GlassFishJakartaEE8/Instances"; // NOI18N
- private static String JAKARTAEE9_INSTANCES_PATH =
"/GlassFishJakartaEE9/Instances"; // NOI18N
- private static String JAKARTAEE91_INSTANCES_PATH =
"/GlassFishJakartaEE91/Instances"; // NOI18N
- private static String JAKARTAEE10_INSTANCES_PATH =
"/GlassFishJakartaEE10/Instances"; // NOI18N
- private static String EE6WC_INSTANCES_PATH = "/GlassFishEE6WC/Instances";
// NOI18N
-
- public static String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude";
//NOI18N
- public static String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N
+ private static final String EE6_INSTANCES_PATH =
"/GlassFishEE6/Instances"; // NOI18N
+ private static final String EE7_INSTANCES_PATH =
"/GlassFishEE7/Instances"; // NOI18N
+ private static final String EE8_INSTANCES_PATH =
"/GlassFishEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE8_INSTANCES_PATH =
"/GlassFishJakartaEE8/Instances"; // NOI18N
+ private static final String JAKARTAEE9_INSTANCES_PATH =
"/GlassFishJakartaEE9/Instances"; // NOI18N
+ private static final String JAKARTAEE91_INSTANCES_PATH =
"/GlassFishJakartaEE91/Instances"; // NOI18N
+ private static final String JAKARTAEE10_INSTANCES_PATH =
"/GlassFishJakartaEE10/Instances"; // NOI18N
+ private static final String EE6WC_INSTANCES_PATH =
"/GlassFishEE6WC/Instances"; // NOI18N
+
+ public static final String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude";
//NOI18N
+ public static final String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; //
NOI18N
Review Comment:
Looking at the signature file and if I remember corretly, this changes the
semantic of the two fields. Before the change users had to lookup the value
from the fields at runtime and thus when linking with a newer module, would get
updated values. This now being `private static final` requires all modules to
be rebuild when the value changes. I would advice against that. A comment
mentioning that might be helpful.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java:
##########
@@ -251,21 +260,21 @@ public GlassfishInstance getGlassfishInstance(String uri)
{
* @param si GlassFish server instance to be added.
*/
public void addServerInstance(GlassfishInstance si) {
- synchronized(instanceMap) {
- try {
- instanceMap.put(si.getDeployerUri(), si);
- activeDisplayNames.add(si.getDisplayName());
- if (instanceMap.size() == 1) { // only need to do if this
first of this type
- RegisteredDDCatalog catalog = getDDCatalog();
- if (null != catalog) {
- catalog.refreshRunTimeDDCatalog(this,
si.getGlassfishRoot());
- }
+ try {
Review Comment:
The code makes assumptions about sychronisation (see line 268 in new code,
line 258 in old code). As the synchronisation was removed, between line 264
(new code) and line 268 (new code) the map might be modified and the the
following code is not executed.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -1114,11 +1121,9 @@ public void stateChanged(final FetchLogEvent event) {
switch (event.getState()) {
case COMPLETED: case FAILED:
FetchLog oldLog = null;
- synchronized (serverInputStreams) {
- FetchLog storedLog = serverInputStreams.get(instance);
- if (this.log.equals(storedLog)) {
- oldLog = serverInputStreams.remove(instance);
- }
+ FetchLog storedLog =
serverInputStreams.computeIfPresent(instance, (key, value) -> value);
+ if (this.log.equals(storedLog)) {
+ oldLog = serverInputStreams.remove(instance);
Review Comment:
Changes semantics(race condition). Before operations in lines 1118-1121 were
atomic, after a different thread might change `serverInputStreams` and/or
`oldLog` while the lines are executed.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java:
##########
@@ -716,8 +711,8 @@ public static void writeInstanceToFile(
/** Configuration changes listener watching <code>domain.xml</code> file.
*/
private final transient DomainXMLChangeListener domainXMLListener;
- private transient InstanceContent ic;
- private transient Lookup localLookup;
+ private final transient InstanceContent ic;
+ private final transient Lookup localLookup;
Review Comment:
`final transient`? This smells funny and I would drop the `transient`.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -135,14 +147,11 @@ private LogViewMgr(final String uri) {
*/
public static LogViewMgr getInstance(String uri) {
LogViewMgr logViewMgr;
- synchronized (instances) {
- WeakReference<LogViewMgr> viewRef = instances.get(uri);
- logViewMgr = viewRef != null ? viewRef.get() : null;
- if(logViewMgr == null) {
- logViewMgr = new LogViewMgr(uri);
- instances.put(uri, new WeakReference<LogViewMgr>(logViewMgr));
- }
- }
+
+ logViewMgr = instances.computeIfAbsent(uri, key ->
+ new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+ ).get();
Review Comment:
This is wrong. The value is a `WeakReference`. Line 140 in the old code
fetch the referenced objects, effectively creating a new hardlink for it. If
that fails (`get` returns `null`), then a new instance is created and a new
`WeakReference` is created.
The supplier fed into `ConcurrentMap#computeIfAbsent` will only be invoked,
if the `WeakReference` object is removed, which is not what was checked before.
##########
enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java:
##########
@@ -1196,27 +1197,25 @@ private static FetchLog getServerLogStream(
final GlassfishInstance instance) {
FetchLog log;
FetchLog deadLog = null;
- synchronized (serverInputStreams) {
- log = serverInputStreams.get(instance);
- if (log != null) {
- if (log instanceof FetchLogPiped) {
- // Log reading task in running state
- if (((FetchLogPiped) log).isRunning()) {
- return log;
- // Log reading task is dead
- } else {
- // Postpone cleanup after synchronized block.
- deadLog = log;
- removeLog(instance);
- }
- } else {
+ log = serverInputStreams.computeIfPresent(instance, (key, value) ->
value);
Review Comment:
Again changed semantics. Code that was atomic before is not anymore.
--
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
