RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()

[Peter Levart]

Hi Ogata,

I think that webrev.02 is safe as you describe. But there's one case 
which now has some overhead. It's a common practice to subclass 
ObjectInputStream and override resloveClass() method and implement 
custom resolving (without calling super.resolveClass()). In such case, 
the cached loader is unnecessarily set-up and then not used. So I'm 
thinking about lazy caching.

For example:
- let public readObject() / readUnshared() entry and exit points just 
clear the cached loader (set it to null).
- let resloveClass() do something like the following at entry:

           CachedLoader cl = cachedLoader;
           Thread curThread = Thread.currentThread();
           ClassLoader loader;
           if (cl == null) {
               loader = latestUserDefinedLoader();
               cl = new CachedLoader(loader, curThread);
           } else if (cl.thread == curThread) {
               loader = cl.loader;
           } else {
               // multi threaded use
               loader = latestUserDefinedLoader();
           }

           // and then...
           return Class.forName(name, false, loader);

What do you think?

Regards, Peter


On 10/13/2017 02:36 PM, Kazunori Ogata wrote:
> Hi Alan,
>
> Alan Bateman <Alan.Bateman at oracle.com> wrote on 2017/10/12 22:17:48:
>
>> This is better but it still not safe. You'll have to atomically set/get
>> the cachedLoader or put it into a thread local to ensure that
>> resolveClass picks up the loader cached by the current thread. A thread
>> local could work too although (needs study) it might need a reference to
>> the OIS to guard against nested deserialization with a different stream.
> Thank you for your review. I fixed the code that does not read the
> cachedLoader atomically when the code tries to update it.
>
> Updated webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.02/
>
>
> The updated code does not use atomic CAS or ThreadLocal.  This code can
> race when the cachedLoader is null, but I think it works correctly because
> a pair of a thread and a class loader (stored in a CachedLoader) is always
> correct regardless of the value of the cachedLoader field, and the thread
> that writes the cachedLoader last resets it to null.  The thread that
> failed to cache a loader simply calls latestUserDefinedLoader(), which is
> the same behavior as the original code.
>
> Considering that multi-thread use of an OIS is rare, I don't want to add
> an overhead of CAS to update the cachedLoader, especially on the POWER
> platform because CAS has high overhead.  Caching loaders in a ThreadLocal
> in each OIS can be more thread safe, but I'm not sure if its memory
> overhead is worth doing so for the rare (and correctly working) case.
>
>
> Regards,
> Ogata
>