8169425: Values computed by a ClassValue should not strongly reference the ClassValue

[Remi Forax]

----- Mail original -----
> De: "Paul Sandoz" <paul.sandoz at oracle.com>
> Cc: "Core-Libs-Dev" <core-libs-dev at openjdk.java.net>
> Envoyé: Mercredi 9 Novembre 2016 17:47:34
> Objet: Re: 8169425: Values computed by a ClassValue should not strongly	reference the ClassValue

> Hi Peter,
> 
> Good point about if such support was added it would break the API and also (with
> Remi) about Ephemerons.
> 
> You are correct in stating the constraints in more detail regarding classes in
> the same loader. However, i think that is going into more detail than I would
> prefer for what i think is an edge case. So I want in general to warn
> developers away from strongly referencing this ClassValue in the computed value
> for any associated class.
> 
> If we do get strong feedback that this is a real problem we could consider
> adding a clever little static method like you suggest, with caveats that the
> computing Function might go away.
> 
> At the moment I would prefer to keep things simple and say as little as
> possible.

I agree with Paul, given the number of people that use ClassValue, i think we should keep the thing simple and let people write their own class on top of ClassValue if they need it.

> 
> Paul.

Rémi

> 
> 
>> On 9 Nov 2016, at 05:15, Peter Levart <peter.levart at gmail.com> wrote:
>> 
>> Hi Paul,
>> 
>> What do you think of introducing a static factory method in ClassValue in
>> addition to your @implNotes. The method would go like this (similar to
>> ThreadLocal.withInitial()):
>> 
>> public abstract class ClassValue<T> {
>> 
>>     /**
>>      * Creates a {@code ClassValue} instance which uses given {@code factory}
>>      * function for computing values associated with classes passed as arguments
>>      * to {@link #get} method. The given {@code factory} function will only be
>>      * <a href="../ref/package-summary.html#reachability"><em>weakly
>>      reachable</em></a>
>>      * from the created ClassValue instance, so one must ensure that is is not
>>      Garbage
>>      * Collected at least until the returned ClassValue is not used any more.
>>      * <p>
>>      * Attempts to use created ClassValue instance to lazily calculate another
>>      * associated value after the given factory function is GCed will result in
>>      * {@link IllegalStateException} being thrown from the {@link #get} method.
>>      *
>>      * @param factory the function to be used to produce values associated with
>>      *                passed-in classes and created ClassValue instance
>>      * @param <T> the type of values associated with created ClassValue instance
>>      * @return new instance of ClassValue, weakly referencing given factory function.
>>      * @since 9
>>      */
>>     public static <T> ClassValue<T> withWeakFactory(
>>         Function<? super Class<?>, T> factory)
>>     {
>>         WeakReference<Function<? super Class<?>, T>> factoryRef =
>>             new WeakReference<>(factory);
>> 
>>         return new ClassValue<T>() {
>>             @Override
>>             protected T computeValue(Class<?> type) {
>>                 Function<? super Class<?>, T> factory = factoryRef.get();
>>                 if (factory == null) {
>>                     throw new IllegalStateException(
>>                         "The value factory function has already been GC(ed).");
>>                 }
>>                 return factory.apply(type);
>>             }
>>         };
>>     }
>> 
>> @implNotes could point to this method with an example...
>> 
>> 
>> Regards, Peter
>> 
>> 
>> On 11/09/2016 01:49 PM, Peter Levart wrote:
>>> Or, better yet, using value factory Function instead of Supplier:
>>> 
>>> 
>>> public class WeakFactoryClassValue<T> extends ClassValue<T> {
>>>     private final WeakReference<Function<? super Class<?>, ? extends T>> factoryRef;
>>> 
>>>     public WeakFactoryClassValue(Function<? super Class<?>, ? extends T> factory) {
>>>         factoryRef = new WeakReference<>(factory);
>>>     }
>>> 
>>>     @Override
>>>     protected T computeValue(Class<?> type) {
>>>         Function<? super Class<?>, ? extends T> factory = factoryRef.get();
>>>         if (factory == null) {
>>>             throw new IllegalStateException("Value factory function has already been GCed");
>>>         }
>>>         return factory.apply(type);
>>>     }
>>> }
>>> 
>>> 
>>> The example would then read:
>>> 
>>> public class MyApp {
>>>     // make VALUE_FACTORY stay at least until MyApp class is alive
>>>     private static final Function<Class<?>, Object> VALUE_FACTORY = clazz ->
>>>     MyApp.CV;
>>> 
>>>     public static final ClassValue<Object> CV =
>>>         new WeakFactoryClassValue<>(VALUE_FACTORY);
>>> 
>>>     public static void main(String[] args) {
>>>         // this is OK
>>>         CV.get(MyApp.class);
>>> 
>>>         // even this is OK, it makes CV reachable from Object.class,
>>>         // but VALUE_FACTORY is only weakly reachable
>>>         CV.get(Object.class);
>>>     }
>>> }
>>> 
>>> 
>>> 
>>> Regards, Peter
>>> 
>>> 
>>> 
>>> On 11/09/2016 01:31 PM, Peter Levart wrote:
>>> > The above situation could be prevented by a special concrete
>>> > ClassValue implementation, provided by the platform (loaded by
>>> > bootstrap CL):
>>> >
>>> > public class WeakSupplierClassValue<T> extends ClassValue<T> {
>>> > private final WeakReference<Supplier<T>> supplierRef;
>>> >
>>> > public WeakSupplierClassValue(Supplier<T> supplier) { supplierRef =
>>> > new WeakReference<>(supplier); }
>>> >
>>> > @Override protected T computeValue(Class<?> type) { Supplier<T>
>>> > supplier = supplierRef.get(); if (supplier == null) { throw new
>>> > IllegalStateException("Supplier has already been GCed"); } return
>>> > supplier.get(); } }
>>> >
>>> >
>>> > ...with such utility class, one could rewrite above example to:
>>> >
>>> > public class MyApp { // make CV_SUPPLIER stay at least until MyApp
>>> > class is alive private static final Supplier<Object> CV_SUPPLIER = ()
>>> > -> MyApp.CV;
>>> >
>>> > public static final ClassValue<Object> CV = new
>>> > WeakSupplierClassValue<>(CV_SUPPLIER);
>>> >
>>> > public static void main(String[] args) { // this is OK
>>> > CV.get(MyApp.class);
>>> >
>>> > // even this is OK, it makes CV reachable from Object.class, // but
>>> > CV_SUPPLIER is only weakly reachable CV.get(Object.class); } }
>>> >
>>> >
>>> > Regards, Peter
>>>