Hi Peter, Thank you for your review and summarizing the change.
Regards, Ogata Peter Levart <peter.lev...@gmail.com> wrote on 2017/10/24 14:11:01: > From: Peter Levart <peter.lev...@gmail.com> > To: Kazunori Ogata <oga...@jp.ibm.com> > Cc: Alan Bateman <alan.bate...@oracle.com>, core-libs-dev@openjdk.java.net > Date: 2017/10/24 14:11 > Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() results in > ObjectInputStream.readObject() > > Hi, > > I think that what Ogata has in webrev.03 is correct and the reasoning > could be as follows: > > - each thread writes to field 'cachedLoader' only from its own set of > values that are distinct from the sets of values that may be written by > any other thread, except null value. > - each thread reads field 'cachedLoader' and can verfy that it has read a > value that belongs to the set of its own written values, or null value (in > other words, a thread can verify that it has read a value that was written > by self or null value). > - reads and writes of own set of non-null values always appear in program > order since they are performed by same thread > - a sequence of writes of own set of non-null values performed by some > thread begins after this thread 1st observes a null value read from the > field and ends before this thread finally writes null value back to the > field. The last write performed by some thread is therefore always a write > of null value. > > No matter how writes performed by a mixture of threads finally hit the > actual field, since each thread that writes to it issues its final write > of null value, the value that eventually ends in the field is null value. > > Does this make sense? > > Regards, Peter > On 10/23/17 22:47, Peter Levart wrote: > Hi Ogata, > > Sorry for late reply. You are absolutely right. Good catch! I missed this > scenario. The criteria for placing the mark (current Thread) on a > cachedLoader must include the check that validates previous value for > later restoration which uses the same criteria. Only in such case will one > thread never restore something that has not been placed by it. And this > guarantees that consumed OIS will never retain a reference to any > ClassLoader or Thread. Your webrev.03 looks good to me. > > Regards, Peter > On 10/17/17 13:48, Kazunori Ogata wrote: > Hi Peter, > > Thank you for your comments and the fix. It's a good idea to mark > cachedLoader with the Thread object. > > > I think we need to check the marking thread of cachedLoader before > updating it. Otherwise, there is a scenario that can leak a CachedLoader > object: > > //1. Thread-A enters readObject() and then call resolveClass() > outerCL-A <- null > cachedLoader <- Thread-A > cachedLoader <- CachedLoader-A > > //2. Thread-B enters readObject() and then call resolveClass() > outerCL-B <- CachedLoader-A > cachedLoader <- Thread-B > cachedLoader <- CachedLoader-B1 > > //3. Thread-B returns from readObject() > cachedLoader is unchanged because outerCL.thread == Thread-A > > //4. Thread-B enters readObject() again and then call resolveClass() > outerCL-B <- CachedLoader-B1 > cachedLoader <- Thread-B > cachedLoader <- CachedLoader-B2 > > //5. Thread-A returns from readObject() > cachedLoader <- null > > //6. Thread-B returns from readObject() > cachedLoader <- CachedLoader-B1 // Because outerCL-B.thread is Thread-B > > > By adding checking before updating the mark, Thread-B won't update > cachedLoader, or it only saves null when race occurs (and always restores > to null on exit). > > > Here is the updated webrev: > http://cr.openjdk.java.net/~horii/8188858/webrev.03/ > > I also made minor changes to reduce the number of invocation of the JNI > method Thread.currentThread(). > > > Regards, > Ogata > > > > From: Peter Levart <peter.lev...@gmail.com> > To: Kazunori Ogata <oga...@jp.ibm.com>, Alan Bateman > <alan.bate...@oracle.com> > Cc: core-libs-dev@openjdk.java.net > Date: 2017/10/16 19:58 > Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() > results in ObjectInputStream.readObject() > > > > 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): > > https://urldefense.proofpoint.com/v2/url? > u=http-3A__cr.openjdk.java.net_-7Eplevart_jdk10-2Ddev_8188858-5FOIS.latestUserDefinedLoader.caching_webrev. > 01_&d=DwIDaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=p- > FJcrbNvnCOLkbIdmQ2tigCrcpdU77tlI2EIdaEcJw&m=PbaGqOdJOR6jMQkXDVYmjn6832m7o0LU2bzwt2awUgQ&s=gKz_rwcTcGIw8JvmRqlg1- > OtjqFNXmIs4oQmIXlF3Wc&e= > > > > Regards, Peter > > > >
