GlassFish modules cleaning and some fixes #6596

pepness · 2023-10-19T23:14:16Z

NetBeans GlassFish module notes:

Use ConcurrentHashMap insted of Collections.synchronizedMap
Use ConcurrentHashMap.newKeySet instead of Collections.synchronizedMap
When using ConcurrentHashMap remove some synchronized blocks when using
atomic and concurrent operations (remove, put, containsKey, and get).
Refactor to computeIfAbsent pattern when possible.
Add javadoc support for JAKARTA EE 9 and JAKARTA EE 10
Add missing validations for Jakarta EE 10
Close resource with try-with-resources
Remove redundant calls to si.getDeployerUri() and this.getClass()
Remove redundant call to some methods getConfigRoot(), getDeployerUri()
Use GlassFishLogger instead of generic glassfish-ee logger
Use target class for logger instead of "glassfish"
Regen sig file because ant check-sigtests-release failed
Fix path to javadocs when creating a new ConfigBuilder
Make final some class instances
Refactor method versionToResourceFilesIndexes()
Fix some javadoc comments
Make final some class instances
Add imports and remove commented code
Add TODO comments
Change inefficient creation of new Array
Remove commented code
Fix some typos

NetBeans Testing:

Verify successful execution of libraries and licenses Ant test
Verify successful execution of Verify Sigtests
Verify successful execution of ant -Dcluster.config=release commit-validation
Verify successful execution of unit tests for modules glassfish.common, glassfish.javaee, glassfish.tooling,
and glassfish.eecommon
Started NetBeans and ensure the log didn't have any ERROR or new WARNINGS
Successfully register GlassFish versions 4, 5, 5.1, 6, 6.2, and 7. Then create a web app for each version
and verify that they work.

-Remove commented code -Make some instance fields final -Fix some typos -Add some imports

atomic and concurrent operations (remove, put, containsKey and get). -Remove redundant call to some methods getConfigRoot(), getDeployerUri()

atomic and concurrent operations (remove, put, and get). -Use ConcurrentHashMap.newKeySet instead of Collections.synchronizedMap -Remove redundant calls to si.getDeployerUri() and this.getClass() -Add a TODO comment -Change inefficient creation of new Array

atomic and concurrent operations (remove, put, and get). -Close resource with try-with-resources -Add imports and remove commented code

-Add TODO comments

- Make final some class instances

- Fix some javadoc comments

atomic and concurrent operations - Add javadoc code support for JAKARTA EE 9 and JAKARTA EE 10 - Initialize `serverVersion` and `builder` inside the constructor - Use `GlassFishLogger` instead of generic `glassfish-ee` logger - Make final some class instances

matthiasblaesing

I found some issues, that should be fixed and I think the changes moving from a HashMap with external synchonization to a ConcurrentHashMap is wrong at least in the commented areas. I think I also saw an instance where now synchronization is invoked on a ConcurrentHashMap, which might or might not work depending on the implementation of the Map.
Oh and we need to hope that noone wanted to store a value with the key being null. With a HashMap that works, a ConcurrentHashMap will blow.

matthiasblaesing · 2023-10-21T18:53:09Z

...se/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java

+    public static final String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
+    public static final String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N


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.

The fields are public, will this also require all modules to be rebuild?

The problem is the "final static". It took me some digging to find the specificiation basis and not just some random note, but this builds it:

https://docs.oracle.com/javase/specs/jls/se8/html/jls-4.html#jls-4.12.4:

A constant variable is a final variable of primitive type or type String that is initialized with a constant expression (§15.28). Whether a variable is a constant variable or not may have implications with respect to class initialization (§12.4.1), binary compatibility (§13.1, §13.4.9), and definite assignment (§16 (Definite Assignment)).

https://docs.oracle.com/javase/specs/jls/se8/html/jls-13.html#jls-13.1 (section 3)

A reference to a field that is a constant variable (§4.12.4) must be resolved at compile time to the value V denoted by the constant variable's initializer.

If such a field is static, then no reference to the field should be present in the code in a binary file, including the class or interface which declared the field. Such a field must always appear to have been initialized (§12.4.2); the default initial value for the field (if different than V) must never be observed.

You can observe this with this project: constant_variable.zip

Unzip and run from the base directory:

mvn clean process-classes; javap -v -p target/classes/eu/doppelhelix/dev/constantvariable/UseSite.class

You see, that the string is evaluated at compile time. The string is loaded from the constant table (ldc call) an directly passed to println from System.out. The output was created by OpenJDK 17 as shipped with Ubuntu, it might be different in your case.

public static void main(java.lang.String[]); descriptor: ([Ljava/lang/String;)V flags: (0x0009) ACC_PUBLIC, ACC_STATIC Code: stack=2, locals=1, args_size=1 0: getstatic #7 // Field java/lang/System.out:Ljava/io/PrintStream; 3: ldc #15 // String Hello World! 5: invokevirtual #17 // Method java/io/PrintStream.println:(Ljava/lang/String;)V 8: return LineNumberTable: line 7: 0 line 8: 8 LocalVariableTable: Start Length Slot Name Signature 0 9 0 args [Ljava/lang/String;

Now open ConstantProvider.java, comment out "Variant 1" and choose one of the other variants, repeat.

This yields a much more complex result:

public static void main(java.lang.String[]); descriptor: ([Ljava/lang/String;)V flags: (0x0009) ACC_PUBLIC, ACC_STATIC Code: stack=2, locals=1, args_size=1 0: getstatic #7 // Field java/lang/System.out:Ljava/io/PrintStream; 3: getstatic #13 // Field eu/doppelhelix/dev/constantvariable/ConstantProvider.WORLD:Ljava/lang/String; 6: invokedynamic #19, 0 // InvokeDynamic #0:makeConcatWithConstants:(Ljava/lang/String;)Ljava/lang/String; 11: invokevirtual #23 // Method java/io/PrintStream.println:(Ljava/lang/String;)V 14: return LineNumberTable: line 7: 0 line 8: 14 LocalVariableTable: Start Length Slot Name Signature 0 15 0 args [Ljava/lang/String; } SourceFile: "UseSite.java" BootstrapMethods: 0: #43 REF_invokeStatic java/lang/invoke/StringConcatFactory.makeConcatWithConstants:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/String;[Ljava/lang/Object;)Ljava/lang/invoke/CallSite; Method arguments: #49 Hello \u0001! InnerClasses: public static final #56= #52 of #54; // Lookup=class java/lang/invoke/MethodHandles$Lookup of class java/lang/invoke/MethodHandles

Now the value of ConstantProvider.WORLD is loaded first (get static #13) and then passed to the invoke dynamic, that then builds the string at runtime.

TL;DR version: Move initialization into a static initializer block and the variables can become final, which being inlined.

matthiasblaesing · 2023-10-21T18:56:37Z

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);


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/GlassfishInstance.java

matthiasblaesing · 2023-10-21T19:03:58Z

enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java

+
+        logViewMgr = instances.computeIfAbsent(uri, key -> 
+                        new WeakReference<LogViewMgr>(new LogViewMgr(uri))
+                    ).get();


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.

The supplier will be executed if the key is absent in the map.

I added some logging to this method and test both versions and the behavior is the same, this method triggers when starting, restarting, and stopping the server or when selecting the option View Domain Server Log:

The first time it runs it will:

search the key(uri) in the map, because is the first time it runs with this GlassFish instance it will return null

compute the value to be inserted with the provided key (uri)

insert the [key, value] entry into the map

return the value which is a WeakReference

execute the .get() method from the returned value

assign the LogViewMgr obtained in step 5

return the LogViewMgr object

INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: uri: [/path/to/app/server/glassfish625/glassfish:/path/to/app/server/glassfish625/glassfish/domains/domain1]deployer:gfv610ee9:localhost:4848 INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): null INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri).get(): null INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): java.lang.ref.WeakReference@766adea5 INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: logViewMgr: org.netbeans.modules.glassfish.common.LogViewMgr@766adea3

The subsequent calls to the same GlassFish instance will log the same:

search the key(uri) in the map, it exists in the map

return the value associated to the key, a WeakReference object

execute the .get() method from the returned value

assign the LogViewMgr obtained in step 3

return the LogViewMgr object

INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: uri: [/path/to/app/server/glassfish625/glassfish:/path/to/app/server/glassfish625/glassfish/domains/domain1]deployer:gfv610ee9:localhost:4848 INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): java.lang.ref.WeakReference@766adea5 INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri).get(): org.netbeans.modules.glassfish.common.LogViewMgr@766adea3 INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: instances.get(uri): java.lang.ref.WeakReference@766adea5 INFO [org.netbeans.modules.glassfish.common.LogViewMgr]: logViewMgr: org.netbeans.modules.glassfish.common.LogViewMgr@766adea3

I tested with two different GlassFish instances (version 6 and 7).

Testing does not help when reasoning about possible synchronisation issues. They have the tendency not to manifest in tests and show in real life. The argument must be based on what is guaranteed by the language.

With this change the following can happen:

Call to LogViewMgr#getInstance with a fixed URI uri1 happens. There is no key for uri1 in the map instances and this the supplier is invoked. It creates a new LogViewMgr, wraps it into a WeakReference, places it in instances and returns it to the caller, which unwraps it using WeakReference#get and returns it.

The system does something with the LogViewMgr instance and at some point removes the last hard reference to it

Garbage collections happens

LogViewMgr#getInstance with a fixed URI uri1 is called again. Now there is a WeakReference for the key uri1, which is returned and WeakReference#get is invoked on that. But because (3) happend you now get null. The code is broken.

There is another race in step 1. To better see this, the code can be split to:

public static LogViewMgr getInstance(String uri) { WeakRef<LogViewMgr> logViewMgrRef = instances.computeIfAbsent(uri, key -> new WeakReference<LogViewMgr>(new LogViewMgr(uri)) ); LogViewMgr logViewMgr = logViewMgrRef.get(); return logViewMgr; }

There is a very short window where the instance created in the supplier is not hard referenced anymore. After it is created and before logViewMgrRef.get() it is possible, that GC happens and the instance is collected. logViewMgrRef.get() will then yield null.

Yes, both these things need fixing. Using new in the arguments to a WeakReference should always be considered an anti-pattern. I mean, the language doesn't even guarantee not to garbage collect an object before the constructor has finished! 😉 The strong reference needs to be held, and the reference needs checking on every retrieval.

Thank you for the explanation, the code example makes it easier to understand.

Will this fix the hard reference issue?

public static LogViewMgr getInstance(String uri) { LogViewMgr logViewMgr; logViewMgr = instances.computeIfAbsent(uri, (String key) -> { LogViewMgr _logViewMgr = new LogViewMgr(uri); WeakReference<LogViewMgr> _viewRef = new WeakReference<>(_logViewMgr); return _viewRef; }).get(); return logViewMgr;

Where is the race problem @matthiasblaesing? The ConcurrentHashMap implementation of computeIfAbsent is atomic and concurrent.
ConcurrentHashMap.computeIfAbsent

The entire method invocation is performed atomically. The supplied function is invoked exactly once per invocation of this method if the key is absent, else not at all. Some attempted update operations on this map by other threads may be blocked while computation is in progress, so the computation should be short and simple.

This will be executed in a synchronized block at a Node level instead of synchronizing the entire map.

Will this fix the hard reference issue?

No. That pretty much amounts to the same code.

This will be executed in a synchronized block at a Node level instead of synchronizing the entire map.

Is there evidence that's a bottleneck?

Where is the race problem @matthiasblaesing? The ConcurrentHashMap implementation of computeIfAbsent is atomic and concurrent.

There are two races, so I don't know which of the two needs more explanation than what was provided.

@neilcsmith-net I agree there is no bottleneck, just using a function that didn't fit the code pattern.

@matthiasblaesing After a long week, now I see the problem: the .get() is out of the safe synchronized block, my bad. Thank you both for the time and explanations.

matthiasblaesing · 2023-10-21T19:06:14Z

enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java

+                    FetchLog storedLog = serverInputStreams.computeIfPresent(instance, (key, value) -> value);
+                    if (this.log.equals(storedLog)) {
+                        oldLog = serverInputStreams.remove(instance);


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.

You are correct.

matthiasblaesing · 2023-10-21T19:07:20Z

enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/LogViewMgr.java

-                        removeLog(instance);
-                    }
-                } else {
+        log = serverInputStreams.computeIfPresent(instance, (key, value) -> value);


Again changed semantics. Code that was atomic before is not anymore.

Again, you are correct. Thanks for the observations.

matthiasblaesing · 2023-10-21T19:11:01Z

...se/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java

-                    if (null != catalog) {
-                        catalog.refreshRunTimeDDCatalog(this, si.getGlassfishRoot());
-                    }
+        try {


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.

You are correct, it needs to be synchronized.

enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java

mbien · 2023-10-23T16:50:41Z

enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java

-                synchronized(delegate) {return delegate.get(key);}
+                return delegate.computeIfPresent((String)key, (k, v) -> v);


same here, this is a get()

The same reason, synchronized just a Node instead of the entire map.

please elaborate. Why is this here needed and in java.util.Properties().get() not?

same question for enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstanceProvider.java

`containsKey` should suffice.

mbien · 2023-10-28T17:45:01Z

what anticipated performance issue is the rewrite in CHM supposed to improve? The access pattern looks like there is probably not much contention happening. Once the instances are in the map, the read operations should be really fast while using synchronization since the critical sections are tiny.

the refactoring does also not get rid of many critical sections, e.g:

        public boolean isEmpty() {
            synchronized(delegate) {return delegate.isEmpty();}
        }

so there would be more work left to finish it.

I usually would see concurrency updates a bit outside the scope of cleanups. Is it worth spending the extra time in review for risky changes when the access pattern is ending up to be get() after short initialization?

pepness added 11 commits October 18, 2023 12:46

-Use target class for logger instead of "glassfish"

f2cd4a7

-Remove commented code -Make some instance fields final -Fix some typos -Add some imports

-Use ConcurrentHashMap and remove some synchronized blocks when using

71952d5

atomic and concurrent operations (remove, put, containsKey and get). -Remove redundant call to some methods getConfigRoot(), getDeployerUri()

-Use ConcurrentHashMap and remove some synchronized blocks when using

e9e0e5a

atomic and concurrent operations (remove, put, and get). -Close resource with try-with-resources -Add imports and remove commented code

-Add missing validations for Jakarta EE 10

fdcbcba

-Add TODO comments

- Use ConcurrentHashMap insted of Collections.synchronizedMap

e030830

- Make final some class instances

- Refactor method versionToResourceFilesIndexes()

08bd396

- Fix some javadoc comments

Fix path to javadocs when creating a new ConfigBuilder

f32bdc5

Refactor to computeIfAbsent pattern.

0ebf2c7

Regen sig file because ant check-sigtests-release failed.

4d66c3f

pepness added Java EE/Jakarta EE [ci] enable enterprise job enterprise [ci] enable enterprise job labels Oct 19, 2023

pepness added this to the NB21 milestone Oct 19, 2023

pepness self-assigned this Oct 19, 2023

matthiasblaesing requested changes Oct 21, 2023

View reviewed changes

mbien reviewed Oct 23, 2023

View reviewed changes

enterprise/glassfish.common/src/org/netbeans/modules/glassfish/common/GlassfishInstance.java Outdated Show resolved Hide resolved

mbien reviewed Oct 23, 2023

View reviewed changes

pepness added 3 commits October 27, 2023 18:19

Re-adding synchronized block, it needs to be linearizable

9dc6810

computeIfPresent extra synchronization should not be necessary,

dd2c2c7

`containsKey` should suffice.

Re-adding synchronized blocks, operations need to be linearizable.

e3247f0

neilcsmith-net modified the milestones: NB21, NB22 Jan 16, 2024

ebarboni modified the milestones: NB22, NB23 Apr 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GlassFish modules cleaning and some fixes #6596

GlassFish modules cleaning and some fixes #6596

pepness commented Oct 19, 2023

matthiasblaesing left a comment

matthiasblaesing Oct 21, 2023

pepness Oct 21, 2023

matthiasblaesing Oct 22, 2023

matthiasblaesing Oct 21, 2023

matthiasblaesing Oct 21, 2023

pepness Oct 22, 2023

matthiasblaesing Oct 22, 2023

neilcsmith-net Oct 23, 2023

pepness Oct 23, 2023

pepness Oct 25, 2023

neilcsmith-net Oct 25, 2023

matthiasblaesing Oct 25, 2023

pepness Oct 28, 2023

matthiasblaesing Oct 21, 2023

pepness Oct 28, 2023

matthiasblaesing Oct 21, 2023

pepness Oct 28, 2023

matthiasblaesing Oct 21, 2023

pepness Oct 25, 2023

mbien Oct 23, 2023

pepness Oct 25, 2023

mbien Oct 28, 2023

mbien commented Oct 28, 2023

		public static final String PRELUDE_DEFAULT_NAME = "GlassFish_v3_Prelude"; //NOI18N
		public static final String EE6WC_DEFAULT_NAME = "GlassFish_Server_3.1"; // NOI18N

		synchronized(delegate) {return delegate.get(key);}
		return delegate.computeIfPresent((String)key, (k, v) -> v);

GlassFish modules cleaning and some fixes #6596

Are you sure you want to change the base?

GlassFish modules cleaning and some fixes #6596

Conversation

pepness commented Oct 19, 2023

matthiasblaesing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbien commented Oct 28, 2023