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

[Kazunori Ogata]

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