Re: RFR: jsr166 jdk10 integration wave 2

[Martin Buchholz]

On Tue, Aug 8, 2017 at 5:21 AM, Peter Levart  wrote:

> Hi Martin,
>
> Just a purely theoretical question...
>
> On 08/08/2017 02:06 AM, Martin Buchholz wrote:
>
>> Need to fix
>>
>>
>> 1. JDK-8185830 
>>
>> ConcurrentSkipListSet.clone() fails with UnsupportedOperationException
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/
>>
>> (It would be nice if we could submit a jdk9 backport now instead of
>> waiting)
>>
>
> Regarding ConcurrentSkipListSet.clone()... The 'm' field is final
> presumably to allow "safe" publication of ConcurrentSkipListSet objects to
> other threads via data races.
>
> clone() O.T.O.H. uses Field.setAccessible(true) on a final field, followed
> with Field.set(), which amounts to Unsafe.putObjectVolatile(). Is this
> equivalent to volatile write? Is it possible for a normal write of a
> reference to a cloned ConcurrentSkipListSet (i.e. publication via data
> race) to bubble up before the volatile write of 'm' field inside the clone
> and consequently allow an observer of a reference to the clone to modify
> the backing map of the original ConcurrentSkipListSet?
>

Yeah, I don't think we have any really good answer for this.  I've lobbied
for "safe publication of non-final fields as if final" for a long time.
Even making the field volatile is not enough in theory (but is in practice)
to make the uninitialized field unobservable.

In theory a fence in the constructor is not enough because you need a fence
before every read as well.  Maybe if you replace every write with a release
write and every read with an acquire read you're OK.

But you also want to keep the "final" for software engineering purposes,
since it's "mostly" final.

Re: RFR: jsr166 jdk10 integration wave 2

[Paul Sandoz]

> On 7 Aug 2017, at 17:06, Martin Buchholz  wrote:
> 
> Need to fix
> 
> 
>   1. JDK-8185830 
> 
> ConcurrentSkipListSet.clone() fails with UnsupportedOperationException
> http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/
> 
> (It would be nice if we could submit a jdk9 backport now instead of waiting)


Oops that is embarrassing, i missed that one in review.

+1

Paul.

Re: RFR: jsr166 jdk10 integration wave 2

[Peter Levart]

On 08/08/2017 02:21 PM, Peter Levart wrote:

Hi Martin,

Just a purely theoretical question...

On 08/08/2017 02:06 AM, Martin Buchholz wrote:

Need to fix


1. JDK-8185830 

ConcurrentSkipListSet.clone() fails with UnsupportedOperationException
http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/

(It would be nice if we could submit a jdk9 backport now instead of 
waiting)


Regarding ConcurrentSkipListSet.clone()... The 'm' field is final 
presumably to allow "safe" publication of ConcurrentSkipListSet 
objects to other threads via data races.


clone() O.T.O.H. uses Field.setAccessible(true) on a final field, 
followed with Field.set(), which amounts to 
Unsafe.putObjectVolatile(). Is this equivalent to volatile write? Is 
it possible for a normal write of a reference to a cloned 
ConcurrentSkipListSet (i.e. publication via data race) to bubble up 
before the volatile write of 'm' field inside the clone and 
consequently allow an observer of a reference to the clone to modify 
the backing map of the original ConcurrentSkipListSet?


Regards, Peter


I know it can't be done any better than that, because 
ConcurrentSkipListSet is not final. If it was final, clone() could use 
private constructor to construct a clone. But the same can be done at 
least for objects of ConcurrentSkipListSet runtime class:


if (getClass() == ConcurrentSkipListSet.class) {
return new ConcurrentSkipListSet(clonedMap);
} else {
// proceed as is
}

Hm...

An alternative would be to use explicit fence before the end of clone()

Regards, Peter

Re: RFR: jsr166 jdk10 integration wave 2

[Peter Levart]

Hi Martin,

Just a purely theoretical question...

On 08/08/2017 02:06 AM, Martin Buchholz wrote:

Need to fix


1. JDK-8185830 

ConcurrentSkipListSet.clone() fails with UnsupportedOperationException
http://cr.openjdk.java.net/~martin/webrevs/openjdk10/jsr166-integration/

(It would be nice if we could submit a jdk9 backport now instead of waiting)


Regarding ConcurrentSkipListSet.clone()... The 'm' field is final 
presumably to allow "safe" publication of ConcurrentSkipListSet objects 
to other threads via data races.


clone() O.T.O.H. uses Field.setAccessible(true) on a final field, 
followed with Field.set(), which amounts to Unsafe.putObjectVolatile(). 
Is this equivalent to volatile write? Is it possible for a normal write 
of a reference to a cloned ConcurrentSkipListSet (i.e. publication via 
data race) to bubble up before the volatile write of 'm' field inside 
the clone and consequently allow an observer of a reference to the clone 
to modify the backing map of the original ConcurrentSkipListSet?


Regards, Peter