Mike Matrigali <[EMAIL PROTECTED]> writes: > I have reviewed and committed the following patch. I have the following > comments which weren't enough to hold up this patch: > > 1) Will you at some point be contributing the performance tests which > you used to verify this fix, and the original test which showed the > problem? I know this is a hard problem, just wondering if you are > working on solving it.
I don't think I can contribute the test that found the problem, as it is part of a larger test framework. I can however make a small test that reproduces the problem and attach it to the JIRA issue. > 2) minor nit - could you try to keep lines under 80 chars. Ok, sorry about that. > 3) I was a little surprised that catch blocks did not do anything > on a released system. Then thinking about it, it seemed that ok > since the added code is not actually using the lock mechanism to > protect anything, but instead just to schedule the enclosed work > more fairly. In fact in jdk1.4 systems the code has to work > without the lock calls at all. I have not idea what kind of > exceptions might happen. The checked exceptions that Method.invoke() might throw are IllegalAccessException, IllegalArgumentException and InvocationTargetException. lock() does not throw exceptions, but unlock() throws IllegalMonitorStateException if the current thread is not the owner of the lock. My reasoning was that this could only happen if someone put a bogus ReentrantLock class in their classpath, which is highly unlikely, and in that case we could just fall back to pre-1.5 behaviour. > The only case that really concerned me > is if lock worked but unlock failed - you may want to think about > that case and see if you should raise an exception. The only case in which unlock() fails, is when lock() has failed or hasn't been invoked. We can easily address this issue by adding hasJava5FairLocks = false to the catch blocks and fall back to the old behaviour. > May be useful to add comments why no exception handling is necessary. I will do that. Thanks for reviewing and committing, Knut Anders
