RFR: jsr166 jdk10 integration wave 2

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR: jsr166 jdk10 integration wave 2

Martin Buchholz-3
Need to fix


   1. JDK-8185830 <https://bugs.openjdk.java.net/browse/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)
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <https://bugs.openjdk.java.net/browse/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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

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 <https://bugs.openjdk.java.net/browse/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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: jsr166 jdk10 integration wave 2

Paul Sandoz
In reply to this post by Martin Buchholz-3

> On 7 Aug 2017, at 17:06, Martin Buchholz <[hidden email]> wrote:
>
> Need to fix
>
>
>   1. JDK-8185830 <https://bugs.openjdk.java.net/browse/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.


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR: jsr166 jdk10 integration wave 2

Martin Buchholz-3
In reply to this post by Peter Levart
On Tue, Aug 8, 2017 at 5:21 AM, Peter Levart <[hidden email]> wrote:

> Hi Martin,
>
> Just a purely theoretical question...
>
> On 08/08/2017 02:06 AM, Martin Buchholz wrote:
>
>> Need to fix
>>
>>
>>     1. JDK-8185830 <https://bugs.openjdk.java.net/browse/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.
Loading...