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

[Peter Levart]

Hi Ogata,

I found a problem in my last suggestion. See below...

On 10/16/2017 11:36 AM, Peter Levart wrote:
>
>
> On 10/16/2017 11:02 AM, Peter Levart wrote:
>> For example:
>> - let public readObject() / readUnshared() entry and exit points just 
>> clear the cached loader (set it to null).
>
> An alternative would be for entry point to save and clear the cached 
> loader while exit point would restore / clear it if it is from correct 
> thread / when the call was not nested. Like the following:
>
> public Object readObject() {
>     CachedLoader outerCL = cachedLoader;
>     cachedLoader = null;
>     try {
>         ...
>     } finally {
>         if (outerCL == null || outerCL.thread == 
> Thread.currentThread()) {
>             // restore/clear cached loader when nested/outer call ends
>             cachedLoader = outerCL;
>         }
>     }
> }
>
> with resolveClass() fragment repeated here for comparison:
>
>           CachedLoader cl = cachedLoader;
>           Thread curThread = Thread.currentThread();
>           ClassLoader loader;
>           if (cl == null) {
>               loader = latestUserDefinedLoader();
>               cachedLoader = 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);
>
>
> There are all sorts of races possible when called concurrently from 
> multiple threads, but the worst consequence is that the loader is not 
> cached. I also think that even in the presence of races, the 
> cachedLoader is eventually cleared when all calls to OIS complete. I 
> couldn't think of a situation where such cached loader would remain 
> hanging off the completed OIS because of races.
>
> Well, there is one such situation but for a different reason. For 
> example, if an OIS subclass is constructed solely to override 
> resolveClass method to make it accessible to custom code (for example, 
> make it public and call super.resolveClass()) in order to provide a 
> utility for resolving classes with the default OIS semantics, but such 
> OIS instance is never used for deserialization itself 
> (readObject()/readUnshared() is never called).
>
> To solve this problem, resolveClass() logic, including lazy caching, 
> should be moved to a private method (resolveClass0()) with protected 
> resolveClass() treated like public readObject()/readUnshared() with 
> before/after treatment of cached loader around delegation to 
> resolveClass0(). All OIS internal uses of resolveClass() should then 
> be redirected to resolveClass0().

Oops, this would not work for subclasses that override resolveClass() 
with custom logic. Hm...

The correct and optimal solution is a little bit more involved, I think. 
Here's what I think should work (did not run any tests yet):

http://cr.openjdk.java.net/~plevart/jdk10-dev/8188858_OIS.latestUserDefinedLoader.caching/webrev.01/


Regards, Peter