Re: RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()
Hi David, Yes I think the code works correctly even if cachedLoader is not volatile. When a thread see a non-volatile value, it is either a Thread or CachedLoader object and the thread can check if it owns the cache. If it does, the thread uses the cache and cleans it up on return. If two or more threads see null, each of them tries to grab the cache by putting its Thread object, but one of them will win. Then, the won thread use the cache and cleans it up on return. I update webrev by adding a few lines of comment as: 2442 // This field is left non-volatile although there is a benign race here. 2443 // The thread that see a non-null value can always check if the cache is for 2444 // the thread, and such thread always cleans up the cache on return. Comments for revising the text (and fixing my poor English) are welcome. Webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.04/ Regards, Ogata From: David Holmes <david.holmes@oracle.com> To: Peter Levart <peter.levart@gmail.com>, Kazunori Ogata <OGATAK@jp.ibm.com> Cc: core-libs-dev@openjdk.java.net Date: 2017/10/24 15:49 Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject() Hi Peter, Ogata, Are you saying this is correct even if cachedLoader is not volatile? If so then that needs to be clearly documented. Cheers, David On 24/10/2017 3:11 PM, Peter Levart wrote:
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:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Eh...
I also made minor changes to reduce the number of invocation of the
JNI
method Thread.currentThread().
Regards, Ogata
From: Peter Levart<peter.levart@gmail.com> To: Kazunori Ogata<OGATAK@jp.ibm.com>, Alan Bateman <Alan.Bateman@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_-7Ep...
Regards, Peter
Hi Ogata, David, On 10/24/17 12:53, Kazunori Ogata wrote:
Hi David,
Yes I think the code works correctly even if cachedLoader is not volatile. When a thread see a non-volatile value, it is either a Thread or CachedLoader object and the thread can check if it owns the cache. If it does, the thread uses the cache and cleans it up on return. If two or more threads see null, each of them tries to grab the cache by putting its Thread object, but one of them will win. Then, the won thread use the cache and cleans it up on return.
It may happen that two or more threads that see null all "win". For example: Thread-A and Thread-B see null 'cachedLoader' field and each writes a reference to its own Thread object to it. In a concurrent call to resolveClass(), both threads issue a read from 'cachedLoader' and each see a reference to a Thread object. Since this is not a volatile field, each thread may see either its own Thread object or the other Thread object. All 4 combinations are allowed. So it may happen that each thread sees its own Thread object, so each thread evaluates its own VM.latestUserDefinedLoader(), wraps it with CachedLoader and writes a reference to 'cachedLoader' field. Both threads then may or may not use its own cached value in subsequent internal calls to resolveClass(), depending on which value of 'cachedLoader' they see. They may see its own value or some other thread's value. Each thread only uses it's own value and ignores other thread's values. What is important it that finally both threads write null to 'cachedLoader' before returning from external call to ObjectInputStream. When non volatile field is accessed by multiple threads, it may behave like volatile field on one end of the spectrum or like a thread-local variable on the other end, but usually something in between. The code behaves correctly for either end or anything in between. It's not that we want only one thread to use the cache. We want two things: - if cached ClassLoader is used by some thread then we must guarantee that it is a ClassLoader that was evaluated (VM.latestUserDefinedLoader()) by the same thread in the same external ObjectInputStream call. This is guaranteed by the fact that we keep the evaluated ClassLoader and the evaluating Thread together in the CachedLoader object and only use the cached ClassLoader if current thread is the same as the one that evaluated it. - if a thread uses cache in some ObjectInputStream call, then the thread must also clean-up the cache to not retain either Thread or ClassLoader objects for more time than needed. This guarantee has been described in my previous email and could be summarized by the following assertion: if some thread writes values to 'cachedLoader' field in some external call to ObjectInputStream, then the last value written by such thread in such call is null value. These two things are all we need to be sure of. And of course, that the cache is effective for the common (and the only valid) case of using ObjectInputStream instance in a single thread. Regards, Peter
I update webrev by adding a few lines of comment as:
2442 // This field is left non-volatile although there is a benign race here. 2443 // The thread that see a non-null value can always check if the cache is for 2444 // the thread, and such thread always cleans up the cache on return.
Comments for revising the text (and fixing my poor English) are welcome.
It's hard to come up with a nice short explanation that is understandable to anyone. I'll try to come up with something tomorrow. Regards, Peter
Webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.04/
Regards, Ogata
From: David Holmes <david.holmes@oracle.com> To: Peter Levart <peter.levart@gmail.com>, Kazunori Ogata <OGATAK@jp.ibm.com> Cc: core-libs-dev@openjdk.java.net Date: 2017/10/24 15:49 Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()
Hi Peter, Ogata,
Are you saying this is correct even if cachedLoader is not volatile? If so then that needs to be clearly documented.
Cheers, David
On 24/10/2017 3:11 PM, Peter Levart wrote:
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:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Eh...
I also made minor changes to reduce the number of invocation of the JNI method Thread.currentThread().
Regards, Ogata
From: Peter Levart<peter.levart@gmail.com> To: Kazunori Ogata<OGATAK@jp.ibm.com>, Alan Bateman <Alan.Bateman@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_-7Ep...
Regards, Peter
Hi Peter, I understand the explanation - thanks. I think some documentation is needed else down the track someone will spot the race and also note the lack of volatile and "fix it". Ogata's comment doesn't quite address things as the volatile-ness does not affect the race that exists. Thanks, David On 25/10/2017 7:17 AM, Peter Levart wrote:
Hi Ogata, David,
On 10/24/17 12:53, Kazunori Ogata wrote:
Hi David,
Yes I think the code works correctly even if cachedLoader is not volatile. When a thread see a non-volatile value, it is either a Thread or CachedLoader object and the thread can check if it owns the cache. If it does, the thread uses the cache and cleans it up on return. If two or more threads see null, each of them tries to grab the cache by putting its Thread object, but one of them will win. Then, the won thread use the cache and cleans it up on return.
It may happen that two or more threads that see null all "win". For example:
Thread-A and Thread-B see null 'cachedLoader' field and each writes a reference to its own Thread object to it. In a concurrent call to resolveClass(), both threads issue a read from 'cachedLoader' and each see a reference to a Thread object. Since this is not a volatile field, each thread may see either its own Thread object or the other Thread object. All 4 combinations are allowed. So it may happen that each thread sees its own Thread object, so each thread evaluates its own VM.latestUserDefinedLoader(), wraps it with CachedLoader and writes a reference to 'cachedLoader' field. Both threads then may or may not use its own cached value in subsequent internal calls to resolveClass(), depending on which value of 'cachedLoader' they see. They may see its own value or some other thread's value. Each thread only uses it's own value and ignores other thread's values. What is important it that finally both threads write null to 'cachedLoader' before returning from external call to ObjectInputStream. When non volatile field is accessed by multiple threads, it may behave like volatile field on one end of the spectrum or like a thread-local variable on the other end, but usually something in between. The code behaves correctly for either end or anything in between.
It's not that we want only one thread to use the cache. We want two things:
- if cached ClassLoader is used by some thread then we must guarantee that it is a ClassLoader that was evaluated (VM.latestUserDefinedLoader()) by the same thread in the same external ObjectInputStream call. This is guaranteed by the fact that we keep the evaluated ClassLoader and the evaluating Thread together in the CachedLoader object and only use the cached ClassLoader if current thread is the same as the one that evaluated it. - if a thread uses cache in some ObjectInputStream call, then the thread must also clean-up the cache to not retain either Thread or ClassLoader objects for more time than needed. This guarantee has been described in my previous email and could be summarized by the following assertion: if some thread writes values to 'cachedLoader' field in some external call to ObjectInputStream, then the last value written by such thread in such call is null value.
These two things are all we need to be sure of. And of course, that the cache is effective for the common (and the only valid) case of using ObjectInputStream instance in a single thread.
Regards, Peter
I update webrev by adding a few lines of comment as:
2442 // This field is left non-volatile although there is a benign race here. 2443 // The thread that see a non-null value can always check if the cache is for 2444 // the thread, and such thread always cleans up the cache on return.
Comments for revising the text (and fixing my poor English) are welcome.
It's hard to come up with a nice short explanation that is understandable to anyone. I'll try to come up with something tomorrow.
Regards, Peter
Webrev:http://cr.openjdk.java.net/~horii/8188858/webrev.04/
Regards, Ogata
From: David Holmes<david.holmes@oracle.com> To: Peter Levart<peter.levart@gmail.com>, Kazunori Ogata <OGATAK@jp.ibm.com> Cc:core-libs-dev@openjdk.java.net Date: 2017/10/24 15:49 Subject: Re: RFR: 8188858: Caching latestUserDefinedLoader() results in ObjectInputStream.readObject()
Hi Peter, Ogata,
Are you saying this is correct even if cachedLoader is not volatile? If so then that needs to be clearly documented.
Cheers, David
On 24/10/2017 3:11 PM, Peter Levart wrote:
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:
https://urldefense.proofpoint.com/v2/url?u=http-3A__cr.openjdk.java.net_-7Eh...
I also made minor changes to reduce the number of invocation of the JNI method Thread.currentThread().
Regards, Ogata
From: Peter Levart<peter.levart@gmail.com> To: Kazunori Ogata<OGATAK@jp.ibm.com>, Alan Bateman <Alan.Bateman@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_-7Ep...
Regards, Peter
On 24/10/2017 11:53, Kazunori Ogata wrote:
:
Comments for revising the text (and fixing my poor English) are welcome.
Webrev: http://cr.openjdk.java.net/~horii/8188858/webrev.04/
I've read through webrev.03 and webrev.04 and they seem correct. Are there are more comments on the implementation? I'd like to get someone in the security area to review this before it is sponsored, the reason is that original patches could be abused and this is security-sensitive area. One comment, or suggestion, is to find a better name for the cachedLoader field, esp. as its value may be a Thread or a CachedLoader object. If CachedLoader were renamed to ReaderContext and cachedLoader changed to something like currentReader then it might be a bit clearer. I'm sure there are better names, the important thing is that future maintainers can quickly understand the code. -Alan
participants (4)
-
Alan Bateman
-
David Holmes
-
Kazunori Ogata
-
Peter Levart