JDK-8145371 ClassCastException thrown in LambdaFormEditor.getInCache

[Martin Buchholz]

On Tue, Jan 9, 2018 at 4:50 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:

>
>
> On 9 Jan 2018, at 15:11, Martin Buchholz <martinrb at google.com> wrote:
>
>
>
> On Tue, Jan 9, 2018 at 2:42 PM, Paul Sandoz <paul.sandoz at oracle.com>
> wrote:
>
>>
>>
>> > On 9 Jan 2018, at 14:20, Martin Buchholz <martinrb at google.com> wrote:
>> >
>> > The memory model is already too hard to reason about, but here the VM
>> can assume that the final fields will never be mutated - yet they are!
>>
>> Because of reflection and Field/AccessibleObject.setAccessible the VM is
>> conservative and does not in general assume final fields are really final.
>> Because of that we miss out on some juicy optimisations. We have made some
>> inroads into limiting the use of setAccessible. If we can deprecate and
>> remove it we can make progress on generally applying "final means final” to
>> all Java code (which also means tackling the case of deserialisation).
>>
>>
> Will there be a principled way to handle "final means final" for
> MethodHandle.form?  Maybe a new annotation that means "mutable BUT compile
> as if final" ?
>
>
>
> Yes, i could imagine in the future removing the final modifier and adding
> the @Stable annotation or some variant of for the more “relaxed" form of
> finality that is currently supported. In fact it might be clearer if we do
> that now (make non-final and add @Stable).
>
>
Well, using @Stable here would be very much undefined

 * It is (currently) undefined what happens if a field annotated as stable
 * is given a third value (by explicitly updating a stable field, a
component of
 * a stable array, or a final stable field via reflection or other means).
 * Since the HotSpot VM promotes a non-null component value to constant, it
may
 * be that the Java memory model would appear to be broken, if such a
constant
 * (the second value of the field) is used as the value of the field even
after
 * the field value has changed (to a third value).


>> > LambdaForm is a mutable class, so publishing it via a plain Unsafe
>> write is a (tiny, hard to detect) data race.  I would feel much more
>> comfortable replacing the Unsafe put with a putVolatile and dropping the
>> fence.  Whenever the form field is read, perhaps it should be explicitly
>> read via a volatile or acquire read for safety.
>>
>> That would incur a cost. j.l.invoke contains code that has carefully
>> arranged interactions with the runtime compilers, this is one of those
>> cases.
>>
>
> (we already have a full fence on writes!)
>
>
> True, but the writes should be rare compared to the reads and as Vladimir
> points out the customisation should not change the functionality.
>

We're changing a "truly" final field holding a mutable object via a data
race.  Subsequently we may randomly mix the cached old object fields and
the new object, and perhaps even uninitialized fields of the latter.  In
theory.


> Paul.
>
> As it stands, MethodHandle.form is published via a data race which seems
> dangerous to me, but may be safe in practice because no one is running Java
> on an early Alpha machine.
>
>