CUSTCOM-76: Fix Thread synchronization in WebappClassloader #4378

rdebusscher · 2019-12-12T13:13:08Z

Description

This is a bug fix

Customer reports occasionally a NullPointerException in org.glassfish.web.loader.WebappClassLoader when starting up his Payara Micro (5.193.1) Application.

Caused by: java.lang.NullPointerException
at org.glassfish.web.loader.WebappClassLoader.findResourceInternalFromJars(WebappClassLoader.java:2908)
at org.glassfish.web.loader.WebappClassLoader.findResourceInternal(WebappClassLoader.java:2761)
at org.glassfish.web.loader.WebappClassLoader.findClassInternal(WebappClassLoader.java:2651)

There is a synchronized access on a non final property causing multithreading issues in the addJar() and findResourceInternalFromJars()

Testing

New tests

Testing Performed

Manual smoke testing with a few customer reperoducers to verify normal classloader behaviour

Test suites executed

Payara Samples

Testing Environment

Oracle 8u181, mvn 3.5.4

Documentation

N/A

Notes for Reviewers

Also added {} to if statements if missing.

Update 16/12

I used to the (already present) jarFilesLock object within the findResourceInternalFromJars method since that one was already protecting the access to property jarFiles.
This means that accessing the jar files is now synchronized with start up but also with shutdown.

dmatej · 2019-12-13T12:44:42Z

appserver/web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java

-import org.apache.naming.resources.WebDirContext;
-import org.apache.naming.resources.Resource;
-import org.apache.naming.resources.ResourceAttributes;
+import org.apache.naming.resources.*;


I would stay with a list of classes if it is less than 11.

That's up for debate - personally I hate using * unless I'm literally using every single class :P

Yes, my experience is that if you use more than 10 classes from the same package in imports, usually the new class asks to be refactored, so finally we end up in the same nice state :)

dmatej · 2019-12-13T12:52:51Z

appserver/web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java

@@ -837,56 +838,58 @@ public synchronized void addJar(String jar, JarFile jarFile, File file)
            logger.log(Level.FINER, "addJar({0})", jar);
        }

-        // See IT 11417
-        super.addURL(getURL(file));
+        synchronized (jarFilesPropertyLock) {


The method is already synchronized, how could this help? Two locks can cause deadlock ... but ... in previous version jarFiles are used also as a lock, so that's the same ...

the method addJar() is synchronized, but other parts of the class requires synchronized access to the property jarFiles. But the synchronization on the method does not synchronize on the property.

In this class, there are synchronization block on 6 different items.

dmatej · 2019-12-13T12:54:58Z

appserver/web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java

@@ -2711,7 +2729,7 @@ protected ResourceEntry findResourceInternal(String name, String path) {
        entry = findResourceInternalFromRepositories(name, path);

        if (entry == null) {
-            synchronized (jarFiles) {


Wouldn't be better to extract the synchronized block to a synchronized method, so it would use the same "default" lock?
Or all accesses to the final field should use own but same lock as you implemented, but I would be afraid of deadlocks ... or use Concurrent implementation if it would make sense (I don't think it does here).

I don't want to change the synchronization fundamentally since there seems a lot of synchronization logic in the code. And changing those things means that we probably break the server functionality.

pdudits · 2019-12-13T13:24:35Z

Were you able to confirm that the race condition is between findResourceInternalFromJars and addJar?

My hypothesis would be that it is race condition between findResourceInternalFromJars and closeJars.

findResourceInternalFromJars confirms that jars are still open by invoking openJars:

Payara/appserver/web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java

Line 2838 in c290512

if (!openJARs()) {

openJars synchronizes on jarFilesLock:

Payara/appserver/web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java

Line 2561 in c290512

synchronized (jarFilesLock) {

But before findResourceInternalFromJars proceeds to accessing the jars (or while it is doing it), closeJars get called, and clears the array:

Payara/appserver/web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java

Line 1929 in c290512

public void closeJARs(boolean force) {

It synchronizes on same lock liken openJars, but that method exited a while ago. So if something needs synchronization, it might be findResourceInternalFromJars itself.

rdebusscher · 2019-12-16T07:17:59Z

@pdudits closeJars is also a possibility, but unlikely since issue occurs during startup of application within a @singleton @startup @PostConstruct method. So runtime is not stopping.

But I'll add it since it can lead to some issues in the future.

pdudits · 2019-12-16T07:26:38Z

@pdudits closeJars is also a possibility, but unlikely since issue occurs during startup of application within a @singleton @startup @PostConstruct method. So runtime is not stopping.

But closeJars is not only called on stopping. Quite the opposite, it's called at the end of startup to fight windows' habit of sticking to file references and preventing their deletion:

https://github.com/rdebusscher/Payara/blob/b1e5172/appserver/web/web-core/src/main/java/org/apache/catalina/core/StandardContext.java#L5788

rdebusscher · 2019-12-16T07:31:54Z

I used to the (already present) jarFilesLock object within the findResourceInternalFromJars method since that one was already protecting the access to property jarFiles.
This means that accessing the jar files is now synchronized with start up but also with shutdown.

pdudits

Looking with "Hide whitespace changes" turned on, the change looks good.

But this one passage from indented part really bothers me:

pdudits · 2019-12-16T07:44:41Z

appserver/web/war-util/src/main/java/org/glassfish/web/loader/WebappClassLoader.java

+                String[] result = new String[paths.length + 1];
+                for (int i = 0; i < paths.length; i++) {
+                    result[i] = paths[i];
+                }
+                result[paths.length] = jar;
+                paths = result;


These 4 idioms for appending arrays are quite disturbing here.

The design might stem from pre-1.4 era, where there was nothing similar to CopyOnWriteArrayList<PathEntry>, and is instead simulated with multiple arrays. But these should at least be refactored into form paths = appendArray(paths, entry). And internally use Arrays.copyOf(arr, arr.length+1).

Correct, but the goal of this PR is not to update some already existing code constructs.

rdebusscher · 2019-12-16T10:26:04Z

But closeJars is not only called on stopping. Quite the opposite, it's called at the end of startup to fight windows' habit of sticking to file references

But jars are reopened immediately again to have the classloader working. So not an efficient part of this old code.

rdebusscher · 2020-01-08T06:40:01Z

Customer verified custom build with this changes and reports issue is fixed. Good to be merged in master

Pandrex247 · 2020-01-08T12:00:07Z

Jenkins test please

CUSTCOM-76: Fix Thread synchronization in WebappClassloader

1cd55a6

rdebusscher requested review from dmatej, Pandrex247 and AlanRoth December 12, 2019 13:13

dmatej reviewed Dec 13, 2019

View reviewed changes

Review comments, simplified synchronization, include also closeJar()

89ccdc4

rdebusscher requested review from dmatej and pdudits December 16, 2019 07:27

pdudits approved these changes Dec 16, 2019

View reviewed changes

Cousjava approved these changes Dec 16, 2019

View reviewed changes

Pandrex247 approved these changes Jan 8, 2020

View reviewed changes

Cousjava merged commit ce2fec4 into payara:master Jan 8, 2020

MattGill98 mentioned this pull request Jan 13, 2020

CUSTCOM-76 WebappClassLoader Synchronisation Test #4430

Merged

rdebusscher deleted the CUSTCOM-76 branch January 20, 2020 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CUSTCOM-76: Fix Thread synchronization in WebappClassloader #4378

CUSTCOM-76: Fix Thread synchronization in WebappClassloader #4378

rdebusscher commented Dec 12, 2019 •

edited

Loading

dmatej Dec 13, 2019

Pandrex247 Dec 13, 2019

dmatej Dec 13, 2019

dmatej Dec 13, 2019

rdebusscher Dec 16, 2019

dmatej Dec 13, 2019 •

edited

Loading

rdebusscher Dec 16, 2019

pdudits commented Dec 13, 2019

rdebusscher commented Dec 16, 2019

pdudits commented Dec 16, 2019

rdebusscher commented Dec 16, 2019

pdudits left a comment

pdudits Dec 16, 2019

rdebusscher Dec 16, 2019

rdebusscher commented Dec 16, 2019

rdebusscher commented Jan 8, 2020

Pandrex247 commented Jan 8, 2020

CUSTCOM-76: Fix Thread synchronization in WebappClassloader #4378

CUSTCOM-76: Fix Thread synchronization in WebappClassloader #4378

Conversation

rdebusscher commented Dec 12, 2019 • edited Loading

Description

Testing

New tests

Testing Performed

Test suites executed

Testing Environment

Documentation

Notes for Reviewers

Update 16/12

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

dmatej Dec 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdudits commented Dec 13, 2019

rdebusscher commented Dec 16, 2019

pdudits commented Dec 16, 2019

rdebusscher commented Dec 16, 2019

pdudits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdebusscher commented Dec 16, 2019

rdebusscher commented Jan 8, 2020

Pandrex247 commented Jan 8, 2020

rdebusscher commented Dec 12, 2019 •

edited

Loading

dmatej Dec 13, 2019 •

edited

Loading