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<Object> keys() {
1165 return map.keys();
1166 }
I wonder if you should use:
public Enumeration<Object> 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 <peter.lev...@gmail.com>
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
