[rfc][icedtea-web] RH976833 ClassLoader deadlock
aazores at redhat.com
Thu Sep 5 07:04:04 PDT 2013
On 09/05/13 07:42, Jiri Vanek wrote:
> On 08/15/2013 10:10 PM, Andrew Azores wrote:
>> * netx/net/sourceforge/jnlp/runtime/JNLPClassLoader.java: (loadClass)
>> made unsynchronized.
>> (available, jarIndexes, classpaths, jarEntries,
>> jarLocationSecurityMap) wrapped in synchronized
>> 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:
 being the site mentioned in the original bug report, and  being a
site I randomly found by Googling for Java test applets. It has *9* on
one page! Excellent for testing this particular scenario :)
>> 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.
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.
Attached is the fix-only version of the patch, refactoring all removed
for the time being.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 10204 bytes
Desc: not available
Url : http://mail.openjdk.java.net/pipermail/distro-pkg-dev/attachments/20130905/1741b788/fix.patch
More information about the distro-pkg-dev