Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Brent, Have you considered increasing the visibility of just the EntrySet constructor from private to default access? This should get rid an extra generated class file if you compare the javac output since you are accessing just the constructor from the enclosing class. Jason From: core-libs-dev on behalf of Brent Christian Sent: Tuesday, May 17, 2016 8:46 PM To: Mandy Chung; David Holmes Cc: core-libs-dev Subject: Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival Hi! Here's the final(?) webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ with the following changes since the last one: * The @hidden JavaDoc tag has been removed. It can't be used due to methods disappearing from the API page. Living with the additional JavaDoc is the best option for now. The situation should be improved by JDK-8157000 (thanks for filing, Mandy). * I also firmed up the doc update regarding iterators (using words from the java.util.concurrent package description of "weakly-consistent"). Line 170. New specdiff views: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-javadoc/Properties.html http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-plain/Properties.html * I did make one real code change, making the new Properties.EntrySet inner class static. Line 1250. * I added a new test of unsynchronized, as suggested by Peter, and expanded coverage to all newly-unsynchronized methods. Thanks, -Brent On 5/13/16 8:01 AM, Mandy Chung wrote: > >> On May 12, 2016, at 5:58 PM, David Holmes wrote: >>> >>> With all of the inherited methods @hidden, it looks like that section >>> is left out altogether. >> >> Okay, so I have to say @hidden seems to me to be seriously flawed! If a >> class has a method that can be called then IMHO that has to be documented in >> that class as either being first defined, overridden, or inherited - it >> can't just say nothing as-if the method were not there! >> >> If we are overriding in a trivial way that does not change the specification >> at all then there should be a simple way to show that - perhaps the "Methods >> inherited from ..." should be split into two parts: method implementations >> inherited from ..., and method specifications inherited from ... ?? >> > > I agree for this case these methods should not be hidden as if it’s not there > (that’s probably carried from the original @treatAsPrivate request). > >> I guess I need to raise this with the javadoc folk :( Is there a mailing >> list for that? > > Can you file a JBS issue? Kumar is on vacation and will talk with him when > he’s back. > > Mandy >
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi Brent, The unsynchronized test looks good. Let's hope that we didn't miss any hidden detail. We'll see how this works out when people get hold of new build containing this change. Regards, Peter On 05/18/2016 03:46 AM, Brent Christian wrote: Hi! Here's the final(?) webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ with the following changes since the last one: * The @hidden JavaDoc tag has been removed. It can't be used due to methods disappearing from the API page. Living with the additional JavaDoc is the best option for now. The situation should be improved by JDK-8157000 (thanks for filing, Mandy). * I also firmed up the doc update regarding iterators (using words from the java.util.concurrent package description of "weakly-consistent"). Line 170. New specdiff views: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-javadoc/Properties.html http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-plain/Properties.html * I did make one real code change, making the new Properties.EntrySet inner class static. Line 1250. * I added a new test of unsynchronized, as suggested by Peter, and expanded coverage to all newly-unsynchronized methods. Thanks, -Brent On 5/13/16 8:01 AM, Mandy Chung wrote: On May 12, 2016, at 5:58 PM, David Holmes wrote: With all of the inherited methods @hidden, it looks like that section is left out altogether. Okay, so I have to say @hidden seems to me to be seriously flawed! If a class has a method that can be called then IMHO that has to be documented in that class as either being first defined, overridden, or inherited - it can't just say nothing as-if the method were not there! If we are overriding in a trivial way that does not change the specification at all then there should be a simple way to show that - perhaps the "Methods inherited from ..." should be split into two parts: method implementations inherited from ..., and method specifications inherited from ... ?? I agree for this case these methods should not be hidden as if it’s not there (that’s probably carried from the original @treatAsPrivate request). I guess I need to raise this with the javadoc folk :( Is there a mailing list for that? Can you file a JBS issue? Kumar is on vacation and will talk with him when he’s back. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
On 18/05/2016 11:46 AM, Brent Christian wrote: Hi! Here's the final(?) webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ with the following changes since the last one: No further comments from me. Thanks, David * The @hidden JavaDoc tag has been removed. It can't be used due to methods disappearing from the API page. Living with the additional JavaDoc is the best option for now. The situation should be improved by JDK-8157000 (thanks for filing, Mandy). * I also firmed up the doc update regarding iterators (using words from the java.util.concurrent package description of "weakly-consistent"). Line 170. New specdiff views: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-javadoc/Properties.html http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-plain/Properties.html * I did make one real code change, making the new Properties.EntrySet inner class static. Line 1250. * I added a new test of unsynchronized, as suggested by Peter, and expanded coverage to all newly-unsynchronized methods. Thanks, -Brent On 5/13/16 8:01 AM, Mandy Chung wrote: On May 12, 2016, at 5:58 PM, David Holmes wrote: With all of the inherited methods @hidden, it looks like that section is left out altogether. Okay, so I have to say @hidden seems to me to be seriously flawed! If a class has a method that can be called then IMHO that has to be documented in that class as either being first defined, overridden, or inherited - it can't just say nothing as-if the method were not there! If we are overriding in a trivial way that does not change the specification at all then there should be a simple way to show that - perhaps the "Methods inherited from ..." should be split into two parts: method implementations inherited from ..., and method specifications inherited from ... ?? I agree for this case these methods should not be hidden as if it’s not there (that’s probably carried from the original @treatAsPrivate request). I guess I need to raise this with the javadoc folk :( Is there a mailing list for that? Can you file a JBS issue? Kumar is on vacation and will talk with him when he’s back. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
> On May 17, 2016, at 6:46 PM, Brent Christian > wrote: > > Hi! > > Here's the final(?) webrev: > http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ +1 Nit: line 132, 134 in PropertiesSerialization.java test are long lines that should be wrapped to next line. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi! Here's the final(?) webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/webrev/ with the following changes since the last one: * The @hidden JavaDoc tag has been removed. It can't be used due to methods disappearing from the API page. Living with the additional JavaDoc is the best option for now. The situation should be improved by JDK-8157000 (thanks for filing, Mandy). * I also firmed up the doc update regarding iterators (using words from the java.util.concurrent package description of "weakly-consistent"). Line 170. New specdiff views: http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-javadoc/Properties.html http://cr.openjdk.java.net/~bchristi/8029891/webrev.6/specdiff-plain/Properties.html * I did make one real code change, making the new Properties.EntrySet inner class static. Line 1250. * I added a new test of unsynchronized, as suggested by Peter, and expanded coverage to all newly-unsynchronized methods. Thanks, -Brent On 5/13/16 8:01 AM, Mandy Chung wrote: On May 12, 2016, at 5:58 PM, David Holmes wrote: With all of the inherited methods @hidden, it looks like that section is left out altogether. Okay, so I have to say @hidden seems to me to be seriously flawed! If a class has a method that can be called then IMHO that has to be documented in that class as either being first defined, overridden, or inherited - it can't just say nothing as-if the method were not there! If we are overriding in a trivial way that does not change the specification at all then there should be a simple way to show that - perhaps the "Methods inherited from ..." should be split into two parts: method implementations inherited from ..., and method specifications inherited from ... ?? I agree for this case these methods should not be hidden as if it’s not there (that’s probably carried from the original @treatAsPrivate request). I guess I need to raise this with the javadoc folk :( Is there a mailing list for that? Can you file a JBS issue? Kumar is on vacation and will talk with him when he’s back. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
> On May 12, 2016, at 5:58 PM, David Holmes wrote: >> >> With all of the inherited methods @hidden, it looks like that section >> is left out altogether. > > Okay, so I have to say @hidden seems to me to be seriously flawed! If a class > has a method that can be called then IMHO that has to be documented in that > class as either being first defined, overridden, or inherited - it can't just > say nothing as-if the method were not there! > > If we are overriding in a trivial way that does not change the specification > at all then there should be a simple way to show that - perhaps the "Methods > inherited from ..." should be split into two parts: method implementations > inherited from ..., and method specifications inherited from ... ?? > I agree for this case these methods should not be hidden as if it’s not there (that’s probably carried from the original @treatAsPrivate request). > I guess I need to raise this with the javadoc folk :( Is there a mailing list > for that? Can you file a JBS issue? Kumar is on vacation and will talk with him when he’s back. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
> On May 12, 2016, at 4:55 PM, Brent Christian > wrote: > > Update to the test, and additional comments: > > http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/ > +1 Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Good idea.
Mandy
> On May 13, 2016, at 12:10 AM, Peter Levart wrote:
>
> Hi Brent,
>
>
> This looks good. It might also be a good idea to add a test that checks this
> new implementation detail (the unsynchronized get) that new code will become
> dependent upon, for example:
>
>
> /*
> * @test
> * @bug 8029891
> * @summary Test that the Properties.get() method does not synchronize any more
> * @run main CheckUnsynchronizedGet
> */
> public class CheckUnsynchronizedGet {
>
>public static void main(String[] args) {
>Properties props = new Properties();
>synchronized (props) {
>props.setProperty("key", "value");
>String value =
>CompletableFuture.supplyAsync(() ->
> props.getProperty("key")).join();
>assert "value".equals(value);
>}
>}
> }
>
> What do you think?
>
> Regards, Peter
>
> On 05/13/2016 01:55 AM, Brent Christian wrote:
>> Update to the test, and additional comments:
>>
>> http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
>>
>> Thanks,
>> -Brent
>>
>> On 5/12/16 4:15 PM, Mandy Chung wrote:
>>>
On May 12, 2016, at 2:44 PM, Brent Christian
wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
>
> This patch looks good.
>
> To help future readers to understand this, it may be better to move:
> 1152 private transient ConcurrentHashMap map =
> 1153 new ConcurrentHashMap<>(8);
>
> to the beginning and add a comment describing what are lock-free and what
> are synchronized (basically some part of your summary below).
>
> Also a comment in Hashtable::defaultWriteHashtable and readHashtable
> methods to mention that they are overridden by Properties.
Good ideas.
> In the GetResource.java test, what is the reason taking out:
> 73 URL u2 =
> Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
>
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I
believe?).
>>>
>>> src/java.base/share/classes/sun/util/resources/CalendarData.properties is
>>> still in java.base. This is due to resource encapsulation. Unnamed module
>>> calling ClassLoader::getResource of a resource in a named module returns
>>> null.
>>>
>>>
I fiddled with @modules a bit, but stopped when I discovered that when
the test deadlocks, it hangs on the first call to getResource() and
doesn't get to this line.
I can look into getting it to load CalendarData again, if you like.
>>>
>>> The launcher and builtin class loader implementation has changed so much
>>> that the code path exercised JDK-6977738 no longer exists. I think
>>> dropping line 73 is okay.
>>>
>>> Mandy
>>>
>
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Note that there is a possible initialization circularity introduced by
this patch, which I think is harmless because:
- ThreadLocalRandom has just recently been enhanced to cope with such
situations - CHM needs ThreadLocalRandom in put() for example, when new
entry is being inserted, TLR invokes
Boolean.getBoolean("java.util.secureRandomSeed") as the last thing in
its static initializer but still works correctly even before this last
part of initializer is finished).
- When an invocation of Boolean.getBoolean ends up in the same
Properties instance (the System.getProperties()) it might be asking for
an entry in the same CHM instance (CHM.get()) which is just being
modified by put(). This amounts to re-entrance of CHM instance, but is
harmless as Boolean.getBoolean is only possibly invoked as the last
thing of various CHM modifications methods when the CHM state is already
consistent (see addCount() invocations in CHM).
Regards, Peter
On 05/13/2016 01:55 AM, Brent Christian wrote:
Update to the test, and additional comments:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
Thanks,
-Brent
On 5/12/16 4:15 PM, Mandy Chung wrote:
On May 12, 2016, at 2:44 PM, Brent Christian
wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free
and what are synchronized (basically some part of your summary below).
Also a comment in Hashtable::defaultWriteHashtable and
readHashtable methods to mention that they are overridden by
Properties.
Good ideas.
In the GetResource.java test, what is the reason taking out:
73 URL u2 =
Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I
believe?).
src/java.base/share/classes/sun/util/resources/CalendarData.properties
is still in java.base. This is due to resource encapsulation.
Unnamed module calling ClassLoader::getResource of a resource in a
named module returns null.
I fiddled with @modules a bit, but stopped when I discovered that
when the test deadlocks, it hangs on the first call to getResource()
and doesn't get to this line.
I can look into getting it to load CalendarData again, if you like.
The launcher and builtin class loader implementation has changed so
much that the code path exercised JDK-6977738 no longer exists. I
think dropping line 73 is okay.
Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi Brent,
This looks good. It might also be a good idea to add a test that checks
this new implementation detail (the unsynchronized get) that new code
will become dependent upon, for example:
/*
* @test
* @bug 8029891
* @summary Test that the Properties.get() method does not synchronize
any more
* @run main CheckUnsynchronizedGet
*/
public class CheckUnsynchronizedGet {
public static void main(String[] args) {
Properties props = new Properties();
synchronized (props) {
props.setProperty("key", "value");
String value =
CompletableFuture.supplyAsync(() ->
props.getProperty("key")).join();
assert "value".equals(value);
}
}
}
What do you think?
Regards, Peter
On 05/13/2016 01:55 AM, Brent Christian wrote:
Update to the test, and additional comments:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
Thanks,
-Brent
On 5/12/16 4:15 PM, Mandy Chung wrote:
On May 12, 2016, at 2:44 PM, Brent Christian
wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free
and what are synchronized (basically some part of your summary below).
Also a comment in Hashtable::defaultWriteHashtable and
readHashtable methods to mention that they are overridden by
Properties.
Good ideas.
In the GetResource.java test, what is the reason taking out:
73 URL u2 =
Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I
believe?).
src/java.base/share/classes/sun/util/resources/CalendarData.properties
is still in java.base. This is due to resource encapsulation.
Unnamed module calling ClassLoader::getResource of a resource in a
named module returns null.
I fiddled with @modules a bit, but stopped when I discovered that
when the test deadlocks, it hangs on the first call to getResource()
and doesn't get to this line.
I can look into getting it to load CalendarData again, if you like.
The launcher and builtin class loader implementation has changed so
much that the code path exercised JDK-6977738 no longer exists. I
think dropping line 73 is okay.
Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi Brent, On 13/05/2016 7:02 AM, Brent Christian wrote: Hi, David On 5/11/16 8:39 PM, David Holmes wrote: On 11/05/2016 7:56 AM, Brent Christian wrote: While good progress was made during the original code review, all of the overridden methods in Properties caused an explosion of unnecessary JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new "@hidden" JavaDoc tag), we can now avoid the additional clutter. The existing javadoc has a section "Methods inherited from java.util.Hashtable" which I can't see in your specdiff - what does that section say about the methods you overrode to delegate to the CHM instance? Are they simply not listed, or does it lie and claim they are inherited, or does it have some new way to present "@hidden" things? Good catch! specdiff didn't pick up that change with --config=javadoc, which is a bit troubling, but it does show up with --config=plain: http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/specdiff-plain/Properties.html With all of the inherited methods @hidden, it looks like that section is left out altogether. Okay, so I have to say @hidden seems to me to be seriously flawed! If a class has a method that can be called then IMHO that has to be documented in that class as either being first defined, overridden, or inherited - it can't just say nothing as-if the method were not there! If we are overriding in a trivial way that does not change the specification at all then there should be a simple way to show that - perhaps the "Methods inherited from ..." should be split into two parts: method implementations inherited from ..., and method specifications inherited from ... ?? I guess I need to raise this with the javadoc folk :( Is there a mailing list for that? while the original deadlock is resolved by all this change, there still exists a deadlock. I should have been clearer. Since this issue was filed, the code paths in question changed a fair bit with jigsaw. Pre-fix, the test still hung, but in different code (NewDeadlock.txt). As Mandy said, with the fix, Properties::get no longer locks on the object, and this test passes. Ah, sorry about the misunderstanding. Thanks, David Thanks, -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Update to the test, and additional comments:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.5/webrev/
Thanks,
-Brent
On 5/12/16 4:15 PM, Mandy Chung wrote:
On May 12, 2016, at 2:44 PM, Brent Christian wrote:
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free and what are
synchronized (basically some part of your summary below).
Also a comment in Hashtable::defaultWriteHashtable and readHashtable methods to
mention that they are overridden by Properties.
Good ideas.
In the GetResource.java test, what is the reason taking out:
73 URL u2 =
Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I believe?).
src/java.base/share/classes/sun/util/resources/CalendarData.properties is still
in java.base. This is due to resource encapsulation. Unnamed module calling
ClassLoader::getResource of a resource in a named module returns null.
I fiddled with @modules a bit, but stopped when I discovered that when the
test deadlocks, it hangs on the first call to getResource() and doesn't get to
this line.
I can look into getting it to load CalendarData again, if you like.
The launcher and builtin class loader implementation has changed so much that
the code path exercised JDK-6977738 no longer exists. I think dropping line 73
is okay.
Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
> On May 12, 2016, at 2:44 PM, Brent Christian
> wrote:
>
> On 5/12/16 11:44 AM, Mandy Chung wrote:
>>
>> This patch looks good.
>>
>> To help future readers to understand this, it may be better to move:
>> 1152 private transient ConcurrentHashMap map =
>> 1153 new ConcurrentHashMap<>(8);
>>
>> to the beginning and add a comment describing what are lock-free and what
>> are synchronized (basically some part of your summary below).
>>
>> Also a comment in Hashtable::defaultWriteHashtable and readHashtable methods
>> to mention that they are overridden by Properties.
>
> Good ideas.
>
>> In the GetResource.java test, what is the reason taking out:
>> 73 URL u2 =
>> Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
>>
>
> It was coming back null and failing the test, I think because
> Jigsaw moved CalendarData into a different module (jdk.localedata, I
> believe?).
src/java.base/share/classes/sun/util/resources/CalendarData.properties is still
in java.base. This is due to resource encapsulation. Unnamed module calling
ClassLoader::getResource of a resource in a named module returns null.
> I fiddled with @modules a bit, but stopped when I discovered that when the
> test deadlocks, it hangs on the first call to getResource() and doesn't get
> to this line.
>
> I can look into getting it to load CalendarData again, if you like.
The launcher and builtin class loader implementation has changed so much that
the code path exercised JDK-6977738 no longer exists. I think dropping line 73
is okay.
Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
On 5/12/16 11:44 AM, Mandy Chung wrote:
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free and what are
synchronized (basically some part of your summary below).
Also a comment in Hashtable::defaultWriteHashtable and readHashtable methods to
mention that they are overridden by Properties.
Good ideas.
In the GetResource.java test, what is the reason taking out:
73 URL u2 =
Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
It was coming back null and failing the test, I think because
Jigsaw moved CalendarData into a different module (jdk.localedata, I
believe?). I fiddled with @modules a bit, but stopped when I discovered
that when the test deadlocks, it hangs on the first call to
getResource() and doesn't get to this line.
I can look into getting it to load CalendarData again, if you like.
Thanks,
-Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi, David On 5/11/16 8:39 PM, David Holmes wrote: On 11/05/2016 7:56 AM, Brent Christian wrote: While good progress was made during the original code review, all of the overridden methods in Properties caused an explosion of unnecessary JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new "@hidden" JavaDoc tag), we can now avoid the additional clutter. The existing javadoc has a section "Methods inherited from java.util.Hashtable" which I can't see in your specdiff - what does that section say about the methods you overrode to delegate to the CHM instance? Are they simply not listed, or does it lie and claim they are inherited, or does it have some new way to present "@hidden" things? Good catch! specdiff didn't pick up that change with --config=javadoc, which is a bit troubling, but it does show up with --config=plain: http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/specdiff-plain/Properties.html With all of the inherited methods @hidden, it looks like that section is left out altogether. while the original deadlock is resolved by all this change, there still exists a deadlock. I should have been clearer. Since this issue was filed, the code paths in question changed a fair bit with jigsaw. Pre-fix, the test still hung, but in different code (NewDeadlock.txt). As Mandy said, with the fix, Properties::get no longer locks on the object, and this test passes. Thanks, -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
> On May 11, 2016, at 8:39 PM, David Holmes wrote: > >> While good progress was made during the original code review, all of the >> overridden methods in Properties caused an explosion of unnecessary >> JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new >> "@hidden" JavaDoc tag), we can now avoid the additional clutter. > > The existing javadoc has a section "Methods inherited from > java.util.Hashtable" which I can't see in your specdiff - what does that > section say about the methods you overrode to delegate to the CHM instance? > Are they simply not listed, or does it lie and claim they are inherited, or > does it have some new way to present "@hidden" things? No javadoc will be generated for @Hidden methods. Kumar can give more details about it. > >> FWIW, the deadlock code path is now different than what is seen in the >> bug report (see [3]). We're now getting hung up on the >> Integer.getInteger() call to get MAX_ARITY on line 60 of >> MethodHandleImpl [4]. > > So while the original deadlock is resolved by all this change, there still > exists a deadlock. Properties::get is now lock-free with this change. The stack trace below is the deadlock before this fix. > I can see from the details that store() locks the properties object and can > lead to MethodHandleNatives.linkCallsite, while in another thread we have > > at java.util.Hashtable.get(java.base@9-ea/Hashtable.java:370) > - waiting to lock <0x0006cff097f0> (a java.util.Properties) > ... > at java.lang.invoke.CallSite.(java.base@9-ea/CallSite.java:230) > at > java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(java.base@9-ea/MethodHandleNatives.java:250) > > I think we have some fundamental initialization order problems that need to > be addressed in a holistic way rather than deadlock by deadlock (or > initialization failure by initialization failure). The startup initialization has been updated significantly. That should clean up a few initialization failures. However, I agree that there are still deadlocks and initialization failures to be addressed. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi Brent,
>
> A new webrev is here:
> http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/
This patch looks good.
To help future readers to understand this, it may be better to move:
1152 private transient ConcurrentHashMap map =
1153 new ConcurrentHashMap<>(8);
to the beginning and add a comment describing what are lock-free and what are
synchronized (basically some part of your summary below).
Also a comment in Hashtable::defaultWriteHashtable and readHashtable methods to
mention that they are overridden by Properties.
In the GetResource.java test, what is the reason taking out:
73 URL u2 =
Thread.currentThread().getContextClassLoader().getResource("sun/util/resources/CalendarData.class”);
Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi Brent, On 11/05/2016 7:56 AM, Brent Christian wrote: Hi! Welcome back to 8029891, "Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java". Our previous discussion is at [1]. Briefly, java.util.Properties isa Hashtable, which synchronizes on itself for any access. System.getProperties() returns the Properties instance accessed by the system, which any application code might synchronize on (that's what the test is doing). System properties are a common way for changing default setting, so it's impractical to expect the class loading code path not to call System.getProperty. The fix is to change Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be (partly) "de-sychronized", and deadlocks avoided. As before I don't have any major concerns with this approach. While good progress was made during the original code review, all of the overridden methods in Properties caused an explosion of unnecessary JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new "@hidden" JavaDoc tag), we can now avoid the additional clutter. The existing javadoc has a section "Methods inherited from java.util.Hashtable" which I can't see in your specdiff - what does that section say about the methods you overrode to delegate to the CHM instance? Are they simply not listed, or does it lie and claim they are inherited, or does it have some new way to present "@hidden" things? Highlights from the previous discussion: * Specifics of where to remove synchronization: generally, read methods will be de-synchronized, while write/bulk-read operations will stay synchronized. * We considered whether we might be able use special-case code just for system properties, instead of a general change to Properties. Because a user is able to supply their own Properties object to System.setProperties(), which is set as the system properties, and current behavior reflects changes made to original Properties object, it would be best to change the Properties class itself. * Don't leak extra add/remove functionality through returned Enumerations. * I said I would check on the synchronization of serialization. writeHashtable() looks okay to me; I expect this is not such a concern for readHashtable(). A new webrev is here: http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/ Other changes since the last webrev I posted: * In readHashtable(), I added a check to validate 'elements', as is done in Hashtable. * I have indicated that Iterators from [keySet|entrySet|values].iterator() no longer fail-fast. While perhaps not strictly necessary, given the "no guarantees" nature of the Hashtable spec, my personal preference is to make it clear that Properties no longer makes a best (or any) effort to throw ConcurrentModificationException. This should not affect any working code (which would not see a CME anyway). Given that Properties are meant to be used as a collection of largely static values, adding additional code to maintain fail-fast behavior is not warranted. FWIW, the deadlock code path is now different than what is seen in the bug report (see [3]). We're now getting hung up on the Integer.getInteger() call to get MAX_ARITY on line 60 of MethodHandleImpl [4]. So while the original deadlock is resolved by all this change, there still exists a deadlock. I can see from the details that store() locks the properties object and can lead to MethodHandleNatives.linkCallsite, while in another thread we have at java.util.Hashtable.get(java.base@9-ea/Hashtable.java:370) - waiting to lock <0x0006cff097f0> (a java.util.Properties) ... at java.lang.invoke.CallSite.(java.base@9-ea/CallSite.java:230) at java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(java.base@9-ea/MethodHandleNatives.java:250) I think we have some fundamental initialization order problems that need to be addressed in a holistic way rather than deadlock by deadlock (or initialization failure by initialization failure). Thanks, David Thanks, -Brent 1. http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033147.html 2. http://cr.openjdk.java.net/~bchristi/8029891/webrev.3/specdiff-plain/Properties.html 3. http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/NewDeadlock.txt 4. http://hg.openjdk.java.net/jdk9/dev/jdk/file/1049321b86cb/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
On 5/10/16 2:56 PM, Brent Christian wrote: While good progress was made during the original code review, all of the overridden methods in Properties caused an explosion of unnecessary JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new "@hidden" JavaDoc tag), we can now avoid the additional clutter. To see @hidden in action, here's what the new specdiff looks like: http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/specdiff/package-summary.html -Brent
RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java - A Revival
Hi! Welcome back to 8029891, "Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java". Our previous discussion is at [1]. Briefly, java.util.Properties isa Hashtable, which synchronizes on itself for any access. System.getProperties() returns the Properties instance accessed by the system, which any application code might synchronize on (that's what the test is doing). System properties are a common way for changing default setting, so it's impractical to expect the class loading code path not to call System.getProperty. The fix is to change Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be (partly) "de-sychronized", and deadlocks avoided. While good progress was made during the original code review, all of the overridden methods in Properties caused an explosion of unnecessary JavaDoc (see specdiff at [2]). With the recent fix of 8073100 (new "@hidden" JavaDoc tag), we can now avoid the additional clutter. Highlights from the previous discussion: * Specifics of where to remove synchronization: generally, read methods will be de-synchronized, while write/bulk-read operations will stay synchronized. * We considered whether we might be able use special-case code just for system properties, instead of a general change to Properties. Because a user is able to supply their own Properties object to System.setProperties(), which is set as the system properties, and current behavior reflects changes made to original Properties object, it would be best to change the Properties class itself. * Don't leak extra add/remove functionality through returned Enumerations. * I said I would check on the synchronization of serialization. writeHashtable() looks okay to me; I expect this is not such a concern for readHashtable(). A new webrev is here: http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/ Other changes since the last webrev I posted: * In readHashtable(), I added a check to validate 'elements', as is done in Hashtable. * I have indicated that Iterators from [keySet|entrySet|values].iterator() no longer fail-fast. While perhaps not strictly necessary, given the "no guarantees" nature of the Hashtable spec, my personal preference is to make it clear that Properties no longer makes a best (or any) effort to throw ConcurrentModificationException. This should not affect any working code (which would not see a CME anyway). Given that Properties are meant to be used as a collection of largely static values, adding additional code to maintain fail-fast behavior is not warranted. FWIW, the deadlock code path is now different than what is seen in the bug report (see [3]). We're now getting hung up on the Integer.getInteger() call to get MAX_ARITY on line 60 of MethodHandleImpl [4]. Thanks, -Brent 1. http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033147.html 2. http://cr.openjdk.java.net/~bchristi/8029891/webrev.3/specdiff-plain/Properties.html 3. http://cr.openjdk.java.net/~bchristi/8029891/webrev.4/NewDeadlock.txt 4. http://hg.openjdk.java.net/jdk9/dev/jdk/file/1049321b86cb/src/java.base/share/classes/java/lang/invoke/MethodHandleImpl.java
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
> On May 15, 2015, at 12:09 PM, Brent Christian > wrote: > > Updated webrev: > > http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/ > > I have restored synchronization to modification methods, and to my > interpretation of "bulk read" operations: > >* forEach >* replaceAll >* store: (synchronized(this) in store0; also, entrySet() returns a > synchronizedSet) >* storeToXML: PropertiesDefaultsHandler.store() synchronizes on the > Properties instance >* propertyNames(): returns an Enumeration not backed by the Properies; > calls (still synchronized) enumerate() >* stringPropertyNames returns a Set not backed by the Properties; calls > (still synchronized) enumerateStringProperties > > These continue to return a synchronized[Set|Collection]: >* keySet >* entrySet >* values The enumerate and enumerateStringProperties methods calls entrySet() to enumerate all entries adding to the given map. The methods are synchronized and it can call map.entrySet directly to avoid the unnecessary synchronization. > > These methods should operate on a coherent snapshot: >* clone >* equals >* hashCode > > I expect these two are only used for debugging. I wonder if we could get > away with removing synchronization: >* list >* toString > list() method calls enumerate() that is synchronized to take a snapshot and it doesn’t need synchronize. It may be useful to leave toString with synchronize that we know the string is a snapshot. Otherwise, looks good. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
Hi, Daniel
You're right, thanks.
The situation is the same for elements(). I've updated these in my
local repo.
I checked through the methods that return a Set/Enumeration/etc, and I
believe there's also an issue with entrySet(). The returned Set should
be backed by the map, and support remove operations, but not add/addAll.
However the Set returned from ConcurrentHashMap.entrySet() *does*
provide add/addAll.
Sadly, there is no "unaddableSet()" in java.util.Collections. AFAICT,
I'll need to add a new inner wrapper class for this. :\
Thanks,
-Brent
On 5/15/15 12:58 PM, Daniel Fuchs wrote:
Hi Brent,
1163 @Override
1164 public Enumeration keys() {
1165 return map.keys();
1166 }
I wonder if you should use:
public Enumeration keys() {
return Collections.enumeration(map.keySet());
}
instead.
ConcurrentHashMap.keys() returns an Enumeration which is also an
Iterator which supports removal of elements, that might have
unintended side effects WRT to concurrency & subclassing.
best regards,
-- daniel
On 15/05/15 21:09, Brent Christian wrote:
On 5/13/15 8:53 AM, Mandy Chung wrote:
On May 12, 2015, at 2:26 PM, Peter Levart
wrote:
On 05/12/2015 10:49 PM, Mandy Chung wrote:
But I think it should be pretty safe to make the
java.util.Properties object override all Hashtable methods and
delegate to internal CMH so that:
- all modification methods + all bulk read methods such as
(keySet().toArray, values.toArray) are made synchronized again
- individual entry read methods (get, containsKey, ...) are not
synchronized.
This way we get the benefits of non-synchronized read access
but the change is hardly observable. In particular since
external synchronization on the Properties object makes guarded
code atomic like it is now and individual entry read accesses
are almost equivalent whether they are synchronized or not and
I would be surprised if any code using Properties for the
purpose they were designed for worked any differently.
I agree that making read-only methods not synchronized while
keeping any method with write-access with synchronized is pretty
safe change and low compatibility risk. On the other hand, would
most of the overrridden methods be synchronized then?
Yes, and that doesn't seem to be a problem. Setting properties is
not very frequent operation and is usually quite localized. Reading
properties is, on the other hand, a very frequent operation
dispersed throughout the code (almost like logging) and therefore
very prone to deadlocks like the one in this issue. I think that by
having an unsynchronized get(Property), most problems with
Properties will be solved - deadlock and performance (scalability)
related.
I’m keen on cleaning up the system properties but it is a bigger
scope as it should take other related enhancement requests into
account that should be something for future. I think what you
propose seems a good solution to fix JDK-8029891 while it doesn’t
completely get us off the deadlock due to user code locking the
Properties object.
Updated webrev:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/
I have restored synchronization to modification methods, and to my
interpretation of "bulk read" operations:
* forEach
* replaceAll
* store: (synchronized(this) in store0; also, entrySet() returns a
synchronizedSet)
* storeToXML: PropertiesDefaultsHandler.store() synchronizes on the
Properties instance
* propertyNames(): returns an Enumeration not backed by the
Properies; calls (still synchronized) enumerate()
* stringPropertyNames returns a Set not backed by the Properties;
calls (still synchronized) enumerateStringProperties
These continue to return a synchronized[Set|Collection]:
* keySet
* entrySet
* values
These methods should operate on a coherent snapshot:
* clone
* equals
* hashCode
I expect these two are only used for debugging. I wonder if we could
get away with removing synchronization:
* list
* toString
I'm still looking through the serialization code, though I suspect some
synchronization should be added.
The new webrev also has the updated test case from Peter.
Thanks,
-Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 16/05/2015 5:09 AM, Brent Christian wrote: On 5/13/15 8:53 AM, Mandy Chung wrote: On May 12, 2015, at 2:26 PM, Peter Levart wrote: On 05/12/2015 10:49 PM, Mandy Chung wrote: But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification methods + all bulk read methods such as (keySet().toArray, values.toArray) are made synchronized again - individual entry read methods (get, containsKey, ...) are not synchronized. This way we get the benefits of non-synchronized read access but the change is hardly observable. In particular since external synchronization on the Properties object makes guarded code atomic like it is now and individual entry read accesses are almost equivalent whether they are synchronized or not and I would be surprised if any code using Properties for the purpose they were designed for worked any differently. I agree that making read-only methods not synchronized while keeping any method with write-access with synchronized is pretty safe change and low compatibility risk. On the other hand, would most of the overrridden methods be synchronized then? Yes, and that doesn't seem to be a problem. Setting properties is not very frequent operation and is usually quite localized. Reading properties is, on the other hand, a very frequent operation dispersed throughout the code (almost like logging) and therefore very prone to deadlocks like the one in this issue. I think that by having an unsynchronized get(Property), most problems with Properties will be solved - deadlock and performance (scalability) related. I’m keen on cleaning up the system properties but it is a bigger scope as it should take other related enhancement requests into account that should be something for future. I think what you propose seems a good solution to fix JDK-8029891 while it doesn’t completely get us off the deadlock due to user code locking the Properties object. Updated webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/ This approach seems reasonable to me. I don't have any specific concerns. Thanks, David - I have restored synchronization to modification methods, and to my interpretation of "bulk read" operations: * forEach * replaceAll * store: (synchronized(this) in store0; also, entrySet() returns a synchronizedSet) * storeToXML: PropertiesDefaultsHandler.store() synchronizes on the Properties instance * propertyNames(): returns an Enumeration not backed by the Properies; calls (still synchronized) enumerate() * stringPropertyNames returns a Set not backed by the Properties; calls (still synchronized) enumerateStringProperties These continue to return a synchronized[Set|Collection]: * keySet * entrySet * values These methods should operate on a coherent snapshot: * clone * equals * hashCode I expect these two are only used for debugging. I wonder if we could get away with removing synchronization: * list * toString I'm still looking through the serialization code, though I suspect some synchronization should be added. The new webrev also has the updated test case from Peter. Thanks, -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
Hi Brent,
1163 @Override
1164 public Enumeration keys() {
1165 return map.keys();
1166 }
I wonder if you should use:
public Enumeration keys() {
return Collections.enumeration(map.keySet());
}
instead.
ConcurrentHashMap.keys() returns an Enumeration which is also an
Iterator which supports removal of elements, that might have
unintended side effects WRT to concurrency & subclassing.
best regards,
-- daniel
On 15/05/15 21:09, Brent Christian wrote:
On 5/13/15 8:53 AM, Mandy Chung wrote:
On May 12, 2015, at 2:26 PM, Peter Levart
wrote:
On 05/12/2015 10:49 PM, Mandy Chung wrote:
But I think it should be pretty safe to make the
java.util.Properties object override all Hashtable methods and
delegate to internal CMH so that:
- all modification methods + all bulk read methods such as
(keySet().toArray, values.toArray) are made synchronized again
- individual entry read methods (get, containsKey, ...) are not
synchronized.
This way we get the benefits of non-synchronized read access
but the change is hardly observable. In particular since
external synchronization on the Properties object makes guarded
code atomic like it is now and individual entry read accesses
are almost equivalent whether they are synchronized or not and
I would be surprised if any code using Properties for the
purpose they were designed for worked any differently.
I agree that making read-only methods not synchronized while
keeping any method with write-access with synchronized is pretty
safe change and low compatibility risk. On the other hand, would
most of the overrridden methods be synchronized then?
Yes, and that doesn't seem to be a problem. Setting properties is
not very frequent operation and is usually quite localized. Reading
properties is, on the other hand, a very frequent operation
dispersed throughout the code (almost like logging) and therefore
very prone to deadlocks like the one in this issue. I think that by
having an unsynchronized get(Property), most problems with
Properties will be solved - deadlock and performance (scalability)
related.
I’m keen on cleaning up the system properties but it is a bigger
scope as it should take other related enhancement requests into
account that should be something for future. I think what you
propose seems a good solution to fix JDK-8029891 while it doesn’t
completely get us off the deadlock due to user code locking the
Properties object.
Updated webrev:
http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/
I have restored synchronization to modification methods, and to my
interpretation of "bulk read" operations:
* forEach
* replaceAll
* store: (synchronized(this) in store0; also, entrySet() returns a
synchronizedSet)
* storeToXML: PropertiesDefaultsHandler.store() synchronizes on the
Properties instance
* propertyNames(): returns an Enumeration not backed by the
Properies; calls (still synchronized) enumerate()
* stringPropertyNames returns a Set not backed by the Properties;
calls (still synchronized) enumerateStringProperties
These continue to return a synchronized[Set|Collection]:
* keySet
* entrySet
* values
These methods should operate on a coherent snapshot:
* clone
* equals
* hashCode
I expect these two are only used for debugging. I wonder if we could
get away with removing synchronization:
* list
* toString
I'm still looking through the serialization code, though I suspect some
synchronization should be added.
The new webrev also has the updated test case from Peter.
Thanks,
-Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 5/13/15 8:53 AM, Mandy Chung wrote: On May 12, 2015, at 2:26 PM, Peter Levart wrote: On 05/12/2015 10:49 PM, Mandy Chung wrote: But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification methods + all bulk read methods such as (keySet().toArray, values.toArray) are made synchronized again - individual entry read methods (get, containsKey, ...) are not synchronized. This way we get the benefits of non-synchronized read access but the change is hardly observable. In particular since external synchronization on the Properties object makes guarded code atomic like it is now and individual entry read accesses are almost equivalent whether they are synchronized or not and I would be surprised if any code using Properties for the purpose they were designed for worked any differently. I agree that making read-only methods not synchronized while keeping any method with write-access with synchronized is pretty safe change and low compatibility risk. On the other hand, would most of the overrridden methods be synchronized then? Yes, and that doesn't seem to be a problem. Setting properties is not very frequent operation and is usually quite localized. Reading properties is, on the other hand, a very frequent operation dispersed throughout the code (almost like logging) and therefore very prone to deadlocks like the one in this issue. I think that by having an unsynchronized get(Property), most problems with Properties will be solved - deadlock and performance (scalability) related. I’m keen on cleaning up the system properties but it is a bigger scope as it should take other related enhancement requests into account that should be something for future. I think what you propose seems a good solution to fix JDK-8029891 while it doesn’t completely get us off the deadlock due to user code locking the Properties object. Updated webrev: http://cr.openjdk.java.net/~bchristi/8029891/webrev.1/ I have restored synchronization to modification methods, and to my interpretation of "bulk read" operations: * forEach * replaceAll * store: (synchronized(this) in store0; also, entrySet() returns a synchronizedSet) * storeToXML: PropertiesDefaultsHandler.store() synchronizes on the Properties instance * propertyNames(): returns an Enumeration not backed by the Properies; calls (still synchronized) enumerate() * stringPropertyNames returns a Set not backed by the Properties; calls (still synchronized) enumerateStringProperties These continue to return a synchronized[Set|Collection]: * keySet * entrySet * values These methods should operate on a coherent snapshot: * clone * equals * hashCode I expect these two are only used for debugging. I wonder if we could get away with removing synchronization: * list * toString I'm still looking through the serialization code, though I suspect some synchronization should be added. The new webrev also has the updated test case from Peter. Thanks, -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 05/13/2015 01:45 AM, Paul Sandoz wrote: On May 12, 2015, at 10:49 PM, Mandy Chung wrote: How likely does existing code do this? A proper way is to call System.setProperty. One pattern I found on System.setProperties is like this to add a system property of key/value pair: Properties props = System.getProperties(); props.put(key, value); System.setProperties(props); More investigation needs to be done (e.g. look at System.setProperties and other system property related APIs and any spec change is needed to be made and the compatibility implication) if we agree that it worths keeping this change local to system properties. FWIW you can see some usages here: http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24lang@System@setProperties%28java.util.Properties%29&k=u Thanks. I skimmed on several safe usages before I suggested to consider doing something for system properties but the first one on the above link and there s another one WriteOnceProperties depending on System.setProperties to replace the system Properties object with the argument directly. Definitely if we want to change any behavior, careful evaluation on the compatibility is required (maybe we can't change it and this is something for the future). Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
> On May 12, 2015, at 2:26 PM, Peter Levart wrote: > > > > On 05/12/2015 10:49 PM, Mandy Chung wrote: >>> But I think it should be pretty safe to make the java.util.Properties >>> object override all Hashtable methods and delegate to internal CMH so that: >>> - all modification methods + all bulk read methods such as >>> (keySet().toArray, values.toArray) are made synchronized again >>> - individual entry read methods (get, containsKey, ...) are not >>> synchronized. >>> >>> This way we get the benefits of non-synchronized read access but the change >>> is hardly observable. In particular since external synchronization on the >>> Properties object makes guarded code atomic like it is now and individual >>> entry read accesses are almost equivalent whether they are synchronized or >>> not and I would be surprised if any code using Properties for the purpose >>> they were designed for worked any differently. >> >> I agree that making read-only methods not synchronized while keeping any >> method with write-access with synchronized is pretty safe change and low >> compatibility risk. On the other hand, would most of the overrridden >> methods be synchronized then? > > Yes, and that doesn't seem to be a problem. Setting properties is not very > frequent operation and is usually quite localized. Reading properties is, on > the other hand, a very frequent operation dispersed throughout the code > (almost like logging) and therefore very prone to deadlocks like the one in > this issue. I think that by having an unsynchronized get(Property), most > problems with Properties will be solved - deadlock and performance > (scalability) related. I’m keen on cleaning up the system properties but it is a bigger scope as it should take other related enhancement requests into account that should be something for future. I think what you propose seems a good solution to fix JDK-8029891 while it doesn’t completely get us off the deadlock due to user code locking the Properties object. Thanks. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On May 12, 2015, at 10:49 PM, Mandy Chung wrote: >> >> Ah, I understand Mandy now. You are talking about using special Properties >> implementation just for system properties. Unfortunately, this is currently >> valid code: >> >> Properties props = new Properties(); >> ... >> System.setProperties(props); >> ... >> props.setProperty(key, value); >> assert System.getProperty(key).equals(value); >> > > > How likely does existing code do this? A proper way is to call > System.setProperty. One pattern I found on System.setProperties is like this > to add a system property of key/value pair: > > Properties props = System.getProperties(); > props.put(key, value); > System.setProperties(props); > > More investigation needs to be done (e.g. look at System.setProperties and > other system property related APIs and any spec change is needed to be made > and the compatibility implication) if we agree that it worths keeping this > change local to system properties. > FWIW you can see some usages here: http://grepcode.com/search/usages?type=method&id=repository.grepcode.com%24java%24root@jdk%24openjdk@7u40-b43@java%24lang@System@setProperties%28java.util.Properties%29&k=u Paul.
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 05/12/2015 10:49 PM, Mandy Chung wrote: But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification methods + all bulk read methods such as (keySet().toArray, values.toArray) are made synchronized again - individual entry read methods (get, containsKey, ...) are not synchronized. This way we get the benefits of non-synchronized read access but the change is hardly observable. In particular since external synchronization on the Properties object makes guarded code atomic like it is now and individual entry read accesses are almost equivalent whether they are synchronized or not and I would be surprised if any code using Properties for the purpose they were designed for worked any differently. I agree that making read-only methods not synchronized while keeping any method with write-access with synchronized is pretty safe change and low compatibility risk. On the other hand, would most of the overrridden methods be synchronized then? Yes, and that doesn't seem to be a problem. Setting properties is not very frequent operation and is usually quite localized. Reading properties is, on the other hand, a very frequent operation dispersed throughout the code (almost like logging) and therefore very prone to deadlocks like the one in this issue. I think that by having an unsynchronized get(Property), most problems with Properties will be solved - deadlock and performance (scalability) related. Regards, Peter Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 05/11/2015 11:41 PM, Peter Levart wrote: On 05/12/2015 07:41 AM, Peter Levart wrote: Taking another look at this deadlock issue and the compatibility concerns, I wonder if we should keep this change as a special implementation for system properties rather than having this change to java.util.Properties class. Properties is a Hashtable which specifies the fast-fail behavior (throwing ConcurrentModificationException for concurrent update). There are other issues specific to system properties we want to clean up (e.g. read-only system property, private system property to JDK but not visible to public etc). Any thought? I like this idea, too. :) One thought: In the current fix, clone() and serialization make use of package-private methods. This could present some difficulties if system properties would use its own Properties subclass that would live outside java.util. -Brent Do you have an example where you would like to access/override one of those methods? They are designed to be a private contract between Properties and Hashtable. Regards, Peter Ah, I understand Mandy now. You are talking about using special Properties implementation just for system properties. Unfortunately, this is currently valid code: Properties props = new Properties(); ... System.setProperties(props); ... props.setProperty(key, value); assert System.getProperty(key).equals(value); How likely does existing code do this? A proper way is to call System.setProperty. One pattern I found on System.setProperties is like this to add a system property of key/value pair: Properties props = System.getProperties(); props.put(key, value); System.setProperties(props); More investigation needs to be done (e.g. look at System.setProperties and other system property related APIs and any spec change is needed to be made and the compatibility implication) if we agree that it worths keeping this change local to system properties. By current semantics, the props object must be installed as new system properties by reference, so later changes to it must be visible. Here, the class of system properties is chosen by user. Perhaps the spec of System.setProperties should be changed (I don't have cycle to think through this). But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification methods + all bulk read methods such as (keySet().toArray, values.toArray) are made synchronized again - individual entry read methods (get, containsKey, ...) are not synchronized. This way we get the benefits of non-synchronized read access but the change is hardly observable. In particular since external synchronization on the Properties object makes guarded code atomic like it is now and individual entry read accesses are almost equivalent whether they are synchronized or not and I would be surprised if any code using Properties for the purpose they were designed for worked any differently. I agree that making read-only methods not synchronized while keeping any method with write-access with synchronized is pretty safe change and low compatibility risk. On the other hand, would most of the overrridden methods be synchronized then? Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 05/12/2015 07:41 AM, Peter Levart wrote: Taking another look at this deadlock issue and the compatibility concerns, I wonder if we should keep this change as a special implementation for system properties rather than having this change to java.util.Properties class. Properties is a Hashtable which specifies the fast-fail behavior (throwing ConcurrentModificationException for concurrent update). There are other issues specific to system properties we want to clean up (e.g. read-only system property, private system property to JDK but not visible to public etc). Any thought? I like this idea, too. :) One thought: In the current fix, clone() and serialization make use of package-private methods. This could present some difficulties if system properties would use its own Properties subclass that would live outside java.util. -Brent Do you have an example where you would like to access/override one of those methods? They are designed to be a private contract between Properties and Hashtable. Regards, Peter Ah, I understand Mandy now. You are talking about using special Properties implementation just for system properties. Unfortunately, this is currently valid code: Properties props = new Properties(); ... System.setProperties(props); ... props.setProperty(key, value); assert System.getProperty(key).equals(value); By current semantics, the props object must be installed as new system properties by reference, so later changes to it must be visible. Here, the class of system properties is chosen by user. But I think it should be pretty safe to make the java.util.Properties object override all Hashtable methods and delegate to internal CMH so that: - all modification methods + all bulk read methods such as (keySet().toArray, values.toArray) are made synchronized again - individual entry read methods (get, containsKey, ...) are not synchronized. This way we get the benefits of non-synchronized read access but the change is hardly observable. In particular since external synchronization on the Properties object makes guarded code atomic like it is now and individual entry read accesses are almost equivalent whether they are synchronized or not and I would be surprised if any code using Properties for the purpose they were designed for worked any differently. Regards, Peter
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 05/12/2015 01:43 AM, Brent Christian wrote: Hi, Mandy. Thanks for having a look. On 5/11/15 12:09 PM, Mandy Chung wrote: ~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load* method to putAll entries in one go after the input reader/stream/xml file is loaded and parsed rather than adding one entry at a time. I also like the idea. Taking another look at this deadlock issue and the compatibility concerns, I wonder if we should keep this change as a special implementation for system properties rather than having this change to java.util.Properties class. Properties is a Hashtable which specifies the fast-fail behavior (throwing ConcurrentModificationException for concurrent update). There are other issues specific to system properties we want to clean up (e.g. read-only system property, private system property to JDK but not visible to public etc). Any thought? I like this idea, too. :) One thought: In the current fix, clone() and serialization make use of package-private methods. This could present some difficulties if system properties would use its own Properties subclass that would live outside java.util. -Brent Do you have an example where you would like to access/override one of those methods? They are designed to be a private contract between Properties and Hashtable. Regards, Peter
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
> On May 11, 2015, at 4:43 PM, Brent Christian > wrote: > > Hi, Mandy. Thanks for having a look. > > On 5/11/15 12:09 PM, Mandy Chung wrote: >> ~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load* >> method to putAll entries in one go after the input reader/stream/xml >> file is loaded and parsed rather than adding one entry at a time. > > I also like the idea. > >> Taking another look at this deadlock issue and the compatibility >> concerns, I wonder if we should keep this change as a special >> implementation for system properties rather than having this change to >> java.util.Properties class. Properties is a Hashtable which specifies >> the fast-fail behavior (throwing ConcurrentModificationException for >> concurrent update). There are other issues specific to system >> properties we want to clean up (e.g. read-only system property, private >> system property to JDK but not visible to public etc). >> >> Any thought? > > I like this idea, too. :) > > One thought: > In the current fix, clone() and serialization make use of package-private > methods. This could present some difficulties if system properties would use > its own Properties subclass that would live outside java.util. It could use the shared secret mechanism sun.misc.SharedSecrets. Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
Hi, Mandy. Thanks for having a look. On 5/11/15 12:09 PM, Mandy Chung wrote: ~/WORK/8029891/plevartI'd like Daniel's suggestion to have the load* method to putAll entries in one go after the input reader/stream/xml file is loaded and parsed rather than adding one entry at a time. I also like the idea. Taking another look at this deadlock issue and the compatibility concerns, I wonder if we should keep this change as a special implementation for system properties rather than having this change to java.util.Properties class. Properties is a Hashtable which specifies the fast-fail behavior (throwing ConcurrentModificationException for concurrent update). There are other issues specific to system properties we want to clean up (e.g. read-only system property, private system property to JDK but not visible to public etc). Any thought? I like this idea, too. :) One thought: In the current fix, clone() and serialization make use of package-private methods. This could present some difficulties if system properties would use its own Properties subclass that would live outside java.util. -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
Hi Brent, On 05/04/2015 09:11 AM, Brent Christian wrote: Hi, Please review this fix, courtesy of Peter Levart (thanks!), that I would like to get in. https://bugs.openjdk.java.net/browse/JDK-8029891 http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/ Thanks for taking this on. I haven't seen an updated webrev yet and I reviewed webrev.0 version. Peter is right that the CheckOverrides.java test doesn't need to match the checked exceptions in the method signature to determine if a new method is missing in Properties class to override. If any checked exception is changed in a public/protected Hashtable method, Properties.java should fail the compilation. W.r.t. whether Properties.load* methods should keep the synchronized, the existing behavior of Properties.load* methods update the Properties object with all entries before any other thread calls other write operation (e.g. setProperty or put). As David pointed out, this patch changes the current behavior that the resulting Properties after the load method call might be different if concurrent modification is done by a separate thread. I'm unsure if this incompatibility is a real concern in practice due to the nature concurrency. In order to have predictable result, an application should have their own lock to ensure the Properties entries are updated in the expected order as far as I can see. On the other hand, I'd like Daniel's suggestion to have the load* method to putAll entries in one go after the input reader/stream/xml file is loaded and parsed rather than adding one entry at a time. Taking another look at this deadlock issue and the compatibility concerns, I wonder if we should keep this change as a special implementation for system properties rather than having this change to java.util.Properties class. Properties is a Hashtable which specifies the fast-fail behavior (throwing ConcurrentModificationException for concurrent update). There are other issues specific to system properties we want to clean up (e.g. read-only system property, private system property to JDK but not visible to public etc). Any thought? Mandy
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 06/05/15 11:41, David Holmes wrote: I don't think you want to de-synchronize the load* methods - you don't want two threads calling load concurrently. But that then raises the problem of concurrent modification while a load is in progress. Synchronization ensures serialization and by removing it you have done more than just avoid deadlocks. I think this needs a more careful examination of the expected/desired concurrent interactions between different methods. It may be that simply not utilizing the synchronized Hashtable methods is sufficient to resolve the deadlock, while still providing reasonable serialization via the existing synchronized Properties methods - or it may not. But allowing concurrent modifications will change behaviour in an unexpected, and incompatible way, in my opinion. David - Hi David, You say: "It may be that simply not utilizing the synchronized Hashtable methods is sufficient to resolve the deadlock, while still providing reasonable serialization via the existing synchronized Properties methods - or it may not". How do you propose to not utilize synchronized Hashtable methods? By not utilizing Properties at all? This may be difficult to achieve as most system configuration is specified as system Properties. What I meant was to continue to delegate to the CHM to replace the inherited Hashtable methods, but to keep the synchronized on the Properties specific methods. What about changing load0 to return a plain HashMap? Then you could transfer the HashMap content into the CHM in one go - that's the only part that really needs to be protected against concurrent modification, isn't it? The PropertiesDefaultHandler could also be changed to use an intermediate HashMap object as well. There would be a small compatibility issue however if some subclass of Properties has some special logic for the case where a property is declared twice (for instance, if it overrides put() to merge values)... best regards, -- daniel
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 6/05/2015 5:57 PM, Peter Levart wrote: On 05/05/2015 03:25 AM, David Holmes wrote: Hi Brent, On 5/05/2015 2:11 AM, Brent Christian wrote: Hi, Please review this fix, courtesy of Peter Levart (thanks!), that I would like to get in. https://bugs.openjdk.java.net/browse/JDK-8029891 http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/ There is some discussion of it in the bug report, starting at 2014-12-31. The problem, as stated by Mandy: "System Properties is a hashtable that synchronizes on itself for any access. Currently System.getProperties returns the Properties instance accessed by the system in which any application code might synchronize on it (that's what the test is doing). The problem reported JDK-6977738 is about Properties.store method that was fixed not to synchronize on this instance. System property is a common way for changing the default setting and so it's impractical to expect the class loading code path not to call System.getProperty." This fix changes java.util.Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be "de-sychronized": all methods inherited from Hashtable are overridden, to remove synchronization, and delegate to the internal CHM. I don't think you want to de-synchronize the load* methods - you don't want two threads calling load concurrently. But that then raises the problem of concurrent modification while a load is in progress. Synchronization ensures serialization and by removing it you have done more than just avoid deadlocks. I think this needs a more careful examination of the expected/desired concurrent interactions between different methods. It may be that simply not utilizing the synchronized Hashtable methods is sufficient to resolve the deadlock, while still providing reasonable serialization via the existing synchronized Properties methods - or it may not. But allowing concurrent modifications will change behaviour in an unexpected, and incompatible way, in my opinion. David - Hi David, You say: "It may be that simply not utilizing the synchronized Hashtable methods is sufficient to resolve the deadlock, while still providing reasonable serialization via the existing synchronized Properties methods - or it may not". How do you propose to not utilize synchronized Hashtable methods? By not utilizing Properties at all? This may be difficult to achieve as most system configuration is specified as system Properties. What I meant was to continue to delegate to the CHM to replace the inherited Hashtable methods, but to keep the synchronized on the Properties specific methods. Cheers, David - So what about taking a more conservative approach by making all "modification" and "bulk" methods synchronized and exposing just single-entry "read-only" methods as not synchronized (mainly Hashatable.get())? Regards, Peter The serialized form is unchanged. An alternative approach considered would be for System.getProperties() to return a duplicate snapshot of the current Properties. This presents a compatibility risk to existing code that keeps a reference to the return value of System.getProperties() and expects to either read new properties added afterwards, or set properties on the cached copy. -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 05/05/2015 03:25 AM, David Holmes wrote: Hi Brent, On 5/05/2015 2:11 AM, Brent Christian wrote: Hi, Please review this fix, courtesy of Peter Levart (thanks!), that I would like to get in. https://bugs.openjdk.java.net/browse/JDK-8029891 http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/ There is some discussion of it in the bug report, starting at 2014-12-31. The problem, as stated by Mandy: "System Properties is a hashtable that synchronizes on itself for any access. Currently System.getProperties returns the Properties instance accessed by the system in which any application code might synchronize on it (that's what the test is doing). The problem reported JDK-6977738 is about Properties.store method that was fixed not to synchronize on this instance. System property is a common way for changing the default setting and so it's impractical to expect the class loading code path not to call System.getProperty." This fix changes java.util.Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be "de-sychronized": all methods inherited from Hashtable are overridden, to remove synchronization, and delegate to the internal CHM. I don't think you want to de-synchronize the load* methods - you don't want two threads calling load concurrently. But that then raises the problem of concurrent modification while a load is in progress. Synchronization ensures serialization and by removing it you have done more than just avoid deadlocks. I think this needs a more careful examination of the expected/desired concurrent interactions between different methods. It may be that simply not utilizing the synchronized Hashtable methods is sufficient to resolve the deadlock, while still providing reasonable serialization via the existing synchronized Properties methods - or it may not. But allowing concurrent modifications will change behaviour in an unexpected, and incompatible way, in my opinion. David - Hi David, You say: "It may be that simply not utilizing the synchronized Hashtable methods is sufficient to resolve the deadlock, while still providing reasonable serialization via the existing synchronized Properties methods - or it may not". How do you propose to not utilize synchronized Hashtable methods? By not utilizing Properties at all? This may be difficult to achieve as most system configuration is specified as system Properties. So what about taking a more conservative approach by making all "modification" and "bulk" methods synchronized and exposing just single-entry "read-only" methods as not synchronized (mainly Hashatable.get())? Regards, Peter The serialized form is unchanged. An alternative approach considered would be for System.getProperties() to return a duplicate snapshot of the current Properties. This presents a compatibility risk to existing code that keeps a reference to the return value of System.getProperties() and expects to either read new properties added afterwards, or set properties on the cached copy. -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
Hi Brent, On 5/05/2015 2:11 AM, Brent Christian wrote: Hi, Please review this fix, courtesy of Peter Levart (thanks!), that I would like to get in. https://bugs.openjdk.java.net/browse/JDK-8029891 http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/ There is some discussion of it in the bug report, starting at 2014-12-31. The problem, as stated by Mandy: "System Properties is a hashtable that synchronizes on itself for any access. Currently System.getProperties returns the Properties instance accessed by the system in which any application code might synchronize on it (that's what the test is doing). The problem reported JDK-6977738 is about Properties.store method that was fixed not to synchronize on this instance. System property is a common way for changing the default setting and so it's impractical to expect the class loading code path not to call System.getProperty." This fix changes java.util.Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be "de-sychronized": all methods inherited from Hashtable are overridden, to remove synchronization, and delegate to the internal CHM. I don't think you want to de-synchronize the load* methods - you don't want two threads calling load concurrently. But that then raises the problem of concurrent modification while a load is in progress. Synchronization ensures serialization and by removing it you have done more than just avoid deadlocks. I think this needs a more careful examination of the expected/desired concurrent interactions between different methods. It may be that simply not utilizing the synchronized Hashtable methods is sufficient to resolve the deadlock, while still providing reasonable serialization via the existing synchronized Properties methods - or it may not. But allowing concurrent modifications will change behaviour in an unexpected, and incompatible way, in my opinion. David - The serialized form is unchanged. An alternative approach considered would be for System.getProperties() to return a duplicate snapshot of the current Properties. This presents a compatibility risk to existing code that keeps a reference to the return value of System.getProperties() and expects to either read new properties added afterwards, or set properties on the cached copy. -Brent
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
Hi Brent,
I think all you need for test is this:
import java.lang.reflect.Method;
import java.lang.reflect.Modifier;
import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.Stream;
public class CheckOverrides {
public static void main(String[] args) {
Set pMethodSignatures =
Stream.of(Properties.class.getDeclaredMethods())
.filter(CheckOverrides::isMethodOfInterest)
.map(MethodSignature::new)
.collect(Collectors.toSet());
Map unoverriddenMethods = new HashMap<>();
for (Class superclass = Properties.class.getSuperclass();
superclass != Object.class;
superclass = superclass.getSuperclass()) {
Stream.of(superclass.getDeclaredMethods())
.filter(CheckOverrides::isMethodOfInterest)
.forEach(m -> unoverriddenMethods.putIfAbsent(new
MethodSignature(m), m));
}
unoverriddenMethods.keySet().removeAll(pMethodSignatures);
if (!unoverriddenMethods.isEmpty()) {
throw new RuntimeException(
"The following methods should be overridden by
Properties class:\n" +
unoverriddenMethods.values().stream()
.map(Method::toString)
.collect(Collectors.joining("\n ", " ", "\n"))
);
}
}
static boolean isMethodOfInterest(Method method) {
int mods = method.getModifiers();
return !Modifier.isStatic(mods) &&
(Modifier.isPublic(mods) || Modifier.isProtected(mods));
}
static class MethodSignature {
final Class returnType;
final String name;
final Class[] parameterTypes;
MethodSignature(Method method) {
this(method.getReturnType(), method.getName(),
method.getParameterTypes());
}
private MethodSignature(Class returnType, String name,
Class[] parameterTypes) {
this.returnType = returnType;
this.name = name;
this.parameterTypes = parameterTypes;
}
@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
MethodSignature that = (MethodSignature) o;
if (!returnType.equals(that.returnType)) return false;
if (!name.equals(that.name)) return false;
return Arrays.equals(parameterTypes, that.parameterTypes);
}
@Override
public int hashCode() {
int result = returnType.hashCode();
result = 31 * result + name.hashCode();
result = 31 * result + Arrays.hashCode(parameterTypes);
return result;
}
}
}
Regards, Peter
On 05/04/2015 08:32 PM, Peter Levart wrote:
On 05/04/2015 06:11 PM, Brent Christian wrote:
Hi,
Please review this fix, courtesy of Peter Levart (thanks!), that I
would like to get in.
https://bugs.openjdk.java.net/browse/JDK-8029891
http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/
There is some discussion of it in the bug report, starting at
2014-12-31.
The problem, as stated by Mandy:
"System Properties is a hashtable that synchronizes on itself for any
access. Currently System.getProperties returns the Properties
instance accessed by the system in which any application code might
synchronize on it (that's what the test is doing). The problem
reported JDK-6977738 is about Properties.store method that was fixed
not to synchronize on this instance. System property is a common way
for changing the default setting and so it's impractical to expect
the class loading code path not to call System.getProperty."
This fix changes java.util.Properties to store its values in an
internal ConcurrentHashMap, ignoring its Hashtable heritage. In this
way, Properties can be "de-sychronized": all methods inherited from
Hashtable are overridden, to remove synchronization, and delegate to
the internal CHM.
The serialized form is unchanged.
An alternative approach considered would be for
System.getProperties() to return a duplicate snapshot of the current
Properties. This presents a compatibility risk to existing code that
keeps a reference to the return value of System.getProperties() and
expects to either read new properties added afterwards, or set
properties on the cached copy.
-Brent
Hi Brent,
The test looks mostly good, but I would refrain from matching the
exception types as part of method signature. Why?
- javac already performs all the necessary checks about declared
exceptions if some method overrides another method
- overriding method can declare a subset and/or subtypes of checked
exceptions of those than overridden method declares and this is OK.
- overriding/overridden methods can declare arbitrary unrelated sets
of unchecked exceptions and this is OK.
Another question i
Re: RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
On 05/04/2015 06:11 PM, Brent Christian wrote: Hi, Please review this fix, courtesy of Peter Levart (thanks!), that I would like to get in. https://bugs.openjdk.java.net/browse/JDK-8029891 http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/ There is some discussion of it in the bug report, starting at 2014-12-31. The problem, as stated by Mandy: "System Properties is a hashtable that synchronizes on itself for any access. Currently System.getProperties returns the Properties instance accessed by the system in which any application code might synchronize on it (that's what the test is doing). The problem reported JDK-6977738 is about Properties.store method that was fixed not to synchronize on this instance. System property is a common way for changing the default setting and so it's impractical to expect the class loading code path not to call System.getProperty." This fix changes java.util.Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be "de-sychronized": all methods inherited from Hashtable are overridden, to remove synchronization, and delegate to the internal CHM. The serialized form is unchanged. An alternative approach considered would be for System.getProperties() to return a duplicate snapshot of the current Properties. This presents a compatibility risk to existing code that keeps a reference to the return value of System.getProperties() and expects to either read new properties added afterwards, or set properties on the cached copy. -Brent Hi Brent, The test looks mostly good, but I would refrain from matching the exception types as part of method signature. Why? - javac already performs all the necessary checks about declared exceptions if some method overrides another method - overriding method can declare a subset and/or subtypes of checked exceptions of those than overridden method declares and this is OK. - overriding/overridden methods can declare arbitrary unrelated sets of unchecked exceptions and this is OK. Another question is whether return type should be considered part of the method signature. For JVM it is, but Java, the language, just matches the method's name and parameter types and javac makes sure that overriding method declarest the same return type or a subtype of overridden method's return type. Well, javac generates two methods in the later case - the one declared in the source code with covariant return type and a synthetic bridge method with overridden method's return type that just calls the declared method. Class.getDeclaredMethods() returns both methods in that case. So either "key" shape would work (one that takes returnType as part of key and one that doesn't). I don't think you need or even want to check that Properties declares all the methods with signatures from all interfaces. Just checking superclass(es) would do, as Properties is not abstract and in order to compile it must implement all abstract methods (either by inheriting from superclass(es) or implementing them itself). Specifically, you would not want to enforce interface default methods to be overridden by Properties if they are not overridden by superclass(es) (as the interface default method can only call other methods in the interface or Object methods and you need not override such method in Properties for Properties to be OK). Regards, Peter
RFR 8029891 : Deadlock detected in java/lang/ClassLoader/deadlock/GetResource.java
Hi, Please review this fix, courtesy of Peter Levart (thanks!), that I would like to get in. https://bugs.openjdk.java.net/browse/JDK-8029891 http://cr.openjdk.java.net/~bchristi/8029891/webrev.0/ There is some discussion of it in the bug report, starting at 2014-12-31. The problem, as stated by Mandy: "System Properties is a hashtable that synchronizes on itself for any access. Currently System.getProperties returns the Properties instance accessed by the system in which any application code might synchronize on it (that's what the test is doing). The problem reported JDK-6977738 is about Properties.store method that was fixed not to synchronize on this instance. System property is a common way for changing the default setting and so it's impractical to expect the class loading code path not to call System.getProperty." This fix changes java.util.Properties to store its values in an internal ConcurrentHashMap, ignoring its Hashtable heritage. In this way, Properties can be "de-sychronized": all methods inherited from Hashtable are overridden, to remove synchronization, and delegate to the internal CHM. The serialized form is unchanged. An alternative approach considered would be for System.getProperties() to return a duplicate snapshot of the current Properties. This presents a compatibility risk to existing code that keeps a reference to the return value of System.getProperties() and expects to either read new properties added afterwards, or set properties on the cached copy. -Brent
