[rfc][icedtea-web] RH976833 ClassLoader deadlock
Jiri Vanek
jvanek at redhat.com
Fri Sep 6 03:08:09 PDT 2013
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.
>
>
> 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 ? :)
> + 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 => ?!!?!?
> + 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.
> + 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.
More information about the distro-pkg-dev
mailing list