Hi Alan, Alan Bateman <[email protected]> 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
