[rfc][icedtea-web] RH976833 ClassLoader deadlock
Andrew Azores
aazores at redhat.com
Fri Sep 6 08:28:19 PDT 2013
On 09/06/2013 06:08 AM, Jiri Vanek wrote:
> On 09/05/2013 04:04 PM, Andrew Azores wrote:
>> On 09/05/13 07:42, Jiri Vanek wrote:
>>> On 08/15/2013 10:10 PM, Andrew Azores wrote:
>>>> Changelog:
>>>>
>>>> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java:
>>>> (loadClass) made unsynchronized.
>>>> (available, jarIndexes, classpaths, jarEntries,
>>>> jarLocationSecurityMap) wrapped in synchronized
>>>> classes.
>>>>
>>>> An earlier change to the classloader made it possible for two
>>>> threads to enter deadlock. This is
>>>
>>> Can we have an reproducer for this? But those are mostly very hard
>>> to reproduce, so do not vaste
>>> to much time on it (no metter that reprodcuer can have random
>>> behaviour as it "can deadlock")
>>>
>>
>> I did spend some time previously trying to put together a reproducer,
>> but I wasn't able to recreate
>> the deadlock offline. The deadlock was affecting every webpage with
>> multiple applets on it, however,
>> at least on my system. The two sites I used as "reproducers" were:
>> [1] http://www.browserspy.dk/java.php
>> [2]
>> http://www.cis.upenn.edu/~matuszek/General/JavaVersionTests/JavaTests.html
>> [1] being the site mentioned in the original bug report, and [2]
>> being a site I randomly found by
>> Googling for Java test applets. It has *9* on one page! Excellent for
>> testing this particular
>> scenario :)
>
> oh yes, it is :)
>
>>
>>>> resolved by not giving the thread which calls loadClass the lock on
>>>> the JNLPClassLoader instance,
>>>> and instead synchronizing the member variables which are accessed
>>>> in loadClass and other methods
>>>> called by loadClasss.
>>>>
>>>> Also made some for-loops into for-each-loops for readability.
>>>
>>>
>>> Sorry for being nitpicker, but please separate refactoring form fix.
>>> Both looks good but the
>>> readability of patch is strongly reduced by the mixture.
>>>
>>> Also I'm fan of "refactoring is good time to do an unittest"
>>> So the best would be for each method, you touch with "for-loops into
>>> fpor-each-loops "
>>> refactoring is an excelent candidate for unittest. When I wrote this
>>> to Adam an year ago, he
>>> stopepd todo this :) So I do not wont to force you to do, but please
>>> think about it.
>>
>> Sure I can separate them out.
>
> thank you.
>>
>> Unittesting on the refactors might be awkward because the methods
>> containing the loop refactors are
>> things like JNLPClassLoader#initializePermissions(), which takes no
>> parameters and has no return
>> value. It just adds values into a private list, which doesn't seem to
>> be publicly exposed in any way
>> either. I'm not really sure how I can go about testing the refactor
>> of the for-loop into a
>> for-each-loop in that kind of situation without doing even more
>> refactoring and changing the
>> interface of the class, just to support being able to unittest a much
>> simpler refactor. TBH I'd
>> rather just leave the slightly-messier looking for-loops in place in
>> this case. If I had done a more
>> thorough refactor with methods being split/extracted or anything like
>> that then unit testing would
>> seem more feasible.
>
> Please, do not abandon issue. When method is absolutely not testable,
> it means that it is wrong.
>
> Eg private void method()
>
> which operates some private fields, can be tested by refactoring into
>
> private static affectedFieldType method(oroginallySource Type) {}
> and
> private void method(){
> //do the job
> affectedFieldType x = method(oroginallySource)
> //affect private field
> oroginallySource.getModifiedBy(x)
> }
>
> And of course by extracting hunks from too long methods.
>
> Of course similar approaches can not always be done, especially in
> class like JNLPClasslaoder.
> So use your judgement to refactor and test what can be, and do not
> vaste time where can not be. And of course to make me believe that yor
> changes on code which can not be tested do no harm ;)
>
> And well.. it affects also this patch too:(
>
>>
>> Attached is the fix-only version of the patch, refactoring all
>> removed for the time being.
>>
>>>
>
> Moreover ok.
>
> General nit - it would be necessary add javadocs why affected fields
> have been made synchronised and why affected methods are no longer
> synchronised.
I'll add these in. I've also provided explanations further down this email.
>>
>>
>> diff --git a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> --- a/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> +++ b/netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java
>> @@ -166,7 +166,7 @@
>> private ArrayList<Permission> runtimePermissions = new
>> ArrayList<Permission>();
>>
>> /** all jars not yet part of classloader or active */
>> - private List<JARDesc> available = new ArrayList<JARDesc>();
>> + private List<JARDesc> available =
>> Collections.synchronizedList(new ArrayList<JARDesc>());
>>
>> /** the jar cert verifier tool to verify our jars */
>> private final JarCertVerifier jcv;
>> @@ -174,17 +174,17 @@
>> private boolean signing = false;
>>
>> /** ArrayList containing jar indexes for various jars available
>> to this classloader */
>> - private ArrayList<JarIndex> jarIndexes = new ArrayList<JarIndex>();
>> + private List<JarIndex> jarIndexes =
>> Collections.synchronizedList(new ArrayList<JarIndex>());
>>
>> /** Set of classpath strings declared in the manifest.mf files */
>> - private Set<String> classpaths = new HashSet<String>();
>> + private Set<String> classpaths = Collections.synchronizedSet(new
>> HashSet<String>());
>>
>> /** File entries in the jar files available to this classloader */
>> - private TreeSet<String> jarEntries = new TreeSet<String>();
>> + private Set<String> jarEntries = Collections.synchronizedSet(new
>> TreeSet<String>());
>>
>> /** Map of specific original (remote) CodeSource Urls to
>> securitydesc */
>> - private HashMap<URL, SecurityDesc> jarLocationSecurityMap =
>> - new HashMap<URL, SecurityDesc>();
>> + private Map<URL, SecurityDesc> jarLocationSecurityMap =
>> + Collections.synchronizedMap(new HashMap<URL,
>> SecurityDesc>());
>>
>> /*Set to prevent once tried-to-get resources to be tried again*/
>> private Set<URL> alreadyTried = Collections.synchronizedSet(new
>> HashSet<URL>());
>> @@ -1173,12 +1173,14 @@
>> for (int i = 0; i < jars.size(); i++) {
>> String part = jars.get(i).getPart();
>>
>> - for (int a = 0; a < available.size(); a++) {
>> - JARDesc jar = available.get(a);
>> + synchronized (available) {
>
> (x)synchronised over available
>> + for (int a = 0; a < available.size(); a++) {
> (y)nit - no for each fix here ? :)
I can start putting the for-each fixes back in if you'd like but that's
what the previous version of the patch in this email thread was... this
one plus for-each fixes.
>> + JARDesc jar = available.get(a);
> (x)getting from available
>>
>> - if (part != null && part.equals(jar.getPart()))
>> - if (!jars.contains(jar))
>> - jars.add(jar);
> (x)now modifying jars => ?!!?!?
"jars" is a param passed into this method, and there are two places this
method is called, both of which are supplying local variables to the
method. I suppose I could synchronize on "jars" and there shouldn't be
any harm done, but I don't think it's necessary at the moment. If you
insist I'll throw those couple of lines into a synchronized (jars)
block. I'm synchronizing on "available" because it's an instance
variable, and I don't want anyone to change its state here while I'm
iterating over it, to be safe. For example consider if "available" has 1
element when I call .size() on it, so the loop condition holds and I
enter the loop body with a = 0. Then I am interrupted. When I resume,
somebody else has mutated "available" and removed the only element, so
it is now empty. Then the call to available.get(0) will throw an
IndexOutOfBounds exception as index >= size.
The reason there was deadlock previously was because the loadClass
method was synchronized, which means it was causing the caller to
acquire the lock on the instance of JNLPClassLoader they were calling
the method on. This was the only place that the instance's lock was
actually used within the class, so this mechanism was apparently being
used to ensure that only one thread entered loadClass() at a time. This
was how the previously unsynchronized instance variables like
"available" were kept safe. However the instance's lock wasn't actually
needed, and later on, the thread holding it would also be waiting for a
lock on, IIRC, the codeBaseLoader. Which was now also waiting on the
lock for the JNLPClassLoader instance, because there's some weird
recursive calls to super and whatnot going on. There was no apparent
solution for causing the two competing threads to acquire/release the
relevant locks in the same order, so the solution had to be to stop
using one of the locks unnecessarily and synchronize on the actual
members themselves that needed to be kept safe.
>> + if (part != null && part.equals(jar.getPart()))
>> + if (!jars.contains(jar))
>> + jars.add(jar);
>> + }
>> }
>> }
>> }
>> @@ -1429,7 +1431,7 @@
>> * classloader, or one of the classloaders for the JNLP file's
>> * extensions.
>> */
>> - public synchronized Class<?> loadClass(String name) throws
>> ClassNotFoundException {
>> + public Class<?> loadClass(String name) throws
>> ClassNotFoundException {
>>
>> Class<?> result = findLoadedClassAll(name);
>>
>> @@ -1457,15 +1459,17 @@
>>
>> // Look in 'Class-Path' as specified in the
>> manifest file
>> try {
>> - for (String classpath: classpaths) {
>> - JARDesc desc;
>> - try {
>> - URL jarUrl = new URL(file.getCodeBase(),
>> classpath);
>> - desc = new JARDesc(jarUrl, null, null,
>> false, true, false, true);
>> - } catch (MalformedURLException mfe) {
>> - throw new ClassNotFoundException(name,
>> mfe);
>> + synchronized (classpaths) {
>> + for (String classpath: classpaths) {
> (y)nit - for each fix appear here ? :)
>> + JARDesc desc;
>> + try {
>> + URL jarUrl = new
>> URL(file.getCodeBase(), classpath);
>> + desc = new JARDesc(jarUrl, null,
>> null, false, true, false, true);
>> + } catch (MalformedURLException mfe) {
>> + throw new
>> ClassNotFoundException(name, mfe);
>> + }
>> + addNewJar(desc);
>> }
>> - addNewJar(desc);
>> }
>>
>> result = loadClassExt(name);
>> @@ -1480,31 +1484,33 @@
>>
>> // Currently this loads jars directly from the
>> site. We cannot cache it because this
>> // call is initiated from within the applet, which
>> does not have disk read/write permissions
>> - for (JarIndex index : jarIndexes) {
>> - // Non-generic code in sun.misc.JarIndex
>> - @SuppressWarnings("unchecked")
>> - LinkedList<String> jarList =
>> index.get(name.replace('.', '/'));
>> + synchronized (jarIndexes) {
>
> Here is simliar synchronisation issue as (x)
>
> But I 'm not sure if it is wrong, or if it will jsut return the deadlock.
Synchronizing over the instance variable jarIndexes because we're about
to iterate over it. Same sort of reasoning as the other example above.
The rest of the variables in this hunk are local to the method so no
need to synchronize on them. The next thing worth looking into are the
calls to other JNLPClassLoader methods, ie addNewJar() and
loadClassExt(). addNewJar() does deal with some of the same instance
variables, eg "available" makes an appearance here again, but we are not
iterating over it. Just adding and later removing. Those two operations
are now atomic since available is a wrapped class by
Collections.synchronizedList(), so it should be harmless to not
synchronize over the whole hunk between those two calls. loadClassExt()
doesn't deal directly with any of the newly-synchronized instance
variables, but it does cause quite a few other method calls. As far as I
can tell, the rest of the patch handles the cases where this might cause
issues, however.
>> + for (JarIndex index : jarIndexes) {
>> + // Non-generic code in sun.misc.JarIndex
>> + @SuppressWarnings("unchecked")
>> + LinkedList<String> jarList =
>> index.get(name.replace('.', '/'));
>>
>> - if (jarList != null) {
>> - for (String jarName : jarList) {
>> - JARDesc desc;
>> - try {
>> - desc = new JARDesc(new
>> URL(file.getCodeBase(), jarName),
>> - null, null, false, true,
>> false, true);
>> - } catch (MalformedURLException mfe) {
>> - throw new ClassNotFoundException(name);
>> - }
>> - try {
>> - addNewJar(desc);
>> - } catch (Exception e) {
>> - if (JNLPRuntime.isDebug()) {
>> - e.printStackTrace();
>> + if (jarList != null) {
>> + for (String jarName : jarList) {
>> + JARDesc desc;
>> + try {
>> + desc = new JARDesc(new
>> URL(file.getCodeBase(), jarName),
>> + null, null, false, true,
>> false, true);
>> + } catch (MalformedURLException mfe) {
>> + throw new
>> ClassNotFoundException(name);
>> + }
>> + try {
>> + addNewJar(desc);
>> + } catch (Exception e) {
>> + if (JNLPRuntime.isDebug()) {
>> + e.printStackTrace();
>> + }
>> }
>> }
>> +
>> + // If it still fails, let it error out
>> + result = loadClassExt(name);
>> }
>> -
>> - // If it still fails, let it error out
>> - result = loadClassExt(name);
>> }
>> }
>> }
>> @@ -1886,21 +1892,23 @@
>>
>> protected SecurityDesc getCodeSourceSecurity(URL source) {
>> SecurityDesc sec=jarLocationSecurityMap.get(source);
>> - if (sec == null && !alreadyTried.contains(source)) {
>> - alreadyTried.add(source);
>> - //try to load the jar which is requesting the
>> permissions, but was NOT downloaded by standard way
>> - if (JNLPRuntime.isDebug()) {
>> - System.out.println("Application is trying to get
>> permissions for " + source.toString() + ", which was not added by
>> standard way. Trying to download and verify!");
>> - }
>> - try {
>> - JARDesc des = new JARDesc(source, null, null, false,
>> false, false, false);
>> - addNewJar(des);
>> - sec = jarLocationSecurityMap.get(source);
>> - } catch (Throwable t) {
>> + synchronized (alreadyTried) {
>> + if (sec == null && !alreadyTried.contains(source)) {
>> + alreadyTried.add(source);
>> + //try to load the jar which is requesting the
>> permissions, but was NOT downloaded by standard way
>> if (JNLPRuntime.isDebug()) {
>> - t.printStackTrace();
>> + System.out.println("Application is trying to get
>> permissions for " + source.toString() + ", which was not added by
>> standard way. Trying to download and verify!");
>> }
>> - sec = null;
>> + try {
>> + JARDesc des = new JARDesc(source, null, null,
>> false, false, false, false);
>> + addNewJar(des);
>> + sec = jarLocationSecurityMap.get(source);
>> + } catch (Throwable t) {
>> + if (JNLPRuntime.isDebug()) {
>> + t.printStackTrace();
>> + }
>> + sec = null;
>> + }
>> }
>> }
>> if (sec == null){
>> @@ -1936,8 +1944,10 @@
>> }
>>
>> // security descriptors
>> - for (URL key : extLoader.jarLocationSecurityMap.keySet()) {
>> - jarLocationSecurityMap.put(key,
>> extLoader.jarLocationSecurityMap.get(key));
>> + synchronized (jarLocationSecurityMap) {
>> + for (URL key : extLoader.jarLocationSecurityMap.keySet()) {
>> + jarLocationSecurityMap.put(key,
>> extLoader.jarLocationSecurityMap.get(key));
>> + }
>> }
>> }
>>
>> @@ -2186,9 +2196,11 @@
>> }
>>
>> // Permissions for all remote hosting urls
>> - for (URL u: jarLocationSecurityMap.keySet()) {
>> - permissions.add(new SocketPermission(u.getHost(),
>> - "connect, accept"));
>> + synchronized (jarLocationSecurityMap) {
>> + for (URL u: jarLocationSecurityMap.keySet()) {
>> + permissions.add(new SocketPermission(u.getHost(),
>> + "connect,
>> accept"));
>> + }
>> }
>>
>> // Permissions for codebase urls (if there is a loader)
>>
>
> Well not easy task to understandt this:)
>
> Nice work anyway!
>
> J.
>
>
Thanks,
--
Andrew A
More information about the distro-pkg-dev
mailing list