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

[Peter Levart]

Hi Remi,


On 11/09/2016 06:29 PM, Remi Forax wrote:
> ----- 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.

That's OK. I don't think such class or static method should be added if 
there is no demand for it. But the point I was trying to make is that 
while people can write such utility class themselves, in order to be 
effective, they would have to deploy it with a class loader that is 
otherwise permanent (that doesn't need to go away) and such that the 
class is visible to application. This means that they can't just bundle 
such class with the application.

Regards, Peter

>> 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