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.bate...@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
