Bug 31903 - WebappClassLoader fails to load class form local repository
Summary: WebappClassLoader fails to load class form local repository
Status: RESOLVED FIXED
Alias: None
Product: Tomcat 5
Classification: Unclassified
Component: Catalina (show other bugs)
Version: 5.5.3
Hardware: All All
: P3 normal (vote)
Target Milestone: ---
Assignee: Tomcat Developers Mailing List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2004-10-26 19:30 UTC by Joe Zhou
Modified: 2005-05-05 13:24 UTC (History)
1 user (show)



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Joe Zhou 2004-10-26 19:30:41 UTC
WebappClassLoader may fail to load class from local repository when multiple
threads are trying to load same class concurrently. 

Need to change method findClassInternal (line 1577 ),  

        if ((entry == null) || (entry.binaryContent == null))
            throw new ClassNotFoundException(name);

to 

        if ((entry == null) || (entry.binaryContent == null &&
entry.loadedClass==null))
            throw new ClassNotFoundException(name);

since after one thread calls defineClass() on the entry, the entry's
binaryContent is set to null, but in this case, the class is already loaded, so
the code should not throw ClassNotFoundException. 

Also in method findClassInternal, following code still use double checked
locking, which is proven broken -  

        if (entry.loadedClass == null) {
            synchronized (this) {
                if (entry.loadedClass == null) {
                    clazz = defineClass(name, entry.binaryContent, 0,
                                        entry.binaryContent.length,
                                        codeSource);
                    entry.loadedClass = clazz;
                    entry.binaryContent = null;
                    entry.source = null;
                    entry.codeBase = null;
                    entry.manifest = null;
                    entry.certificates = null;
                } else {
                    clazz = entry.loadedClass;
                }
            }
        } else {
            clazz = entry.loadedClass;
        }
Comment 1 Remy Maucherat 2004-10-26 23:37:20 UTC
Are you reporting an actual problem, or is it just reading of the source code ?

The first part could make sense: I added entry.binaryContent = null after the
rest, so the condition might be wrong.
Comment 2 Joe Zhou 2004-10-27 14:26:10 UTC
This is a reproducible real problem.

Our server uses tomcat 5.0 as a servlet engine. When it runs multiple worker
threads to execute a JavaScritpt (using Rhino) on the server side, it will
occasionally but consistently throw NoClassDefFoundError on a class in Rhino
library, which is located in the WEB-INF\lib directory. 

We eventually figure out if two threads start to load same class concurrently
for WEB-INF/lib, one of them will fail if 

a) the first thread manages to call defineClass on the resource entry and set
the binaryContent to null; 

b) when the second adds its entry to the HashMap, it will find an entry for that
 class already exists, so it will return the existing entry (the one whose
binaryContent is null); 

For the second part, it has been reported by some other people. Please see the
following link

http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html

It would be better to replace the code with single checked locking and make it
thread-safe. 
Comment 3 Remy Maucherat 2004-10-27 14:30:31 UTC
You're not supposed to fork threads in your webapp.

As for the double check thing, try to post productive stuff, thanks.
Comment 4 Jess Holle 2004-10-27 14:33:25 UTC
It's well and good that you're "not supposed to fork threads", but let's get
real -- this is something that code needs to do more often than the strict
letter of J2EE lets on.
Comment 5 Joe Zhou 2004-10-27 14:54:26 UTC
"You're not supposed to fork threads in your webapp."

Oh, this is the simpler fix, thanks.
Comment 6 Remy Maucherat 2004-10-27 15:14:38 UTC
The J2EE spec forbids it, and experience shows it can cause many tricky problems.

The change for entry.loadedClass == null is good, since I added the
entry.binaryContent = null without seeing that the check would need an update.
This was introduced in 5.0.3.
Comment 7 Joe Zhou 2004-10-27 16:20:41 UTC
Thanks Remy. It is forbidden to create threads in ejb container, and it is not
recommended to spawn threads in servlet container, but don't recall this is
strictly forbidden by the servlet spec (I might be wrong here).