On 04/28/2015 10:13 AM, Daniel Fuchs wrote:
On 28/04/15 09:59, Peter Levart wrote:
On 04/27/2015 10:05 PM, David Holmes wrote:
The patch proposes to use a Reantrant lock to deal with
configurations changes in reset() and readConfiguration(),
and avoids lock contention in initializeGlobalHandlers()
http://cr.openjdk.java.net/~dfuchs/webrev_8077846/webrev.00/
How early in VM startup can logging be initialized and used? I just
wonder whether the use of ReentrantLock changes that at all?
The initialization will not happen before the first class calls
Logger.getLogger() or LogManager.getLogManager().
In particular, it does not happen when the LogManager.class is loaded
(that is: it no longer happens as part of the class static
initialization).
I see Alan replied to that question as well...
I'm just wondering what anyone might be running with a customized core
class that uses logging, and which may now fail due to the different
requirements.
But then I just recalled that we used to use ReentrantLock via
ConcurrentHashMap in Class, so introducing it early is not likely to
cause any issue.
Cheers,
David
Hi David, Daniel,
CHM in JDK8 is not using ReentrantLock. But in JDK7, it is used, yes.
But anyway, is there a special reason to use ReentrantLock here in
LogManager instead of Java built-in monitor lock? It is reentrant too.
Hi Peter,
That's one of the possibility I was envisaging - but I didn't feel very
comfortable with synchronizing the whole method bodies.
Hi Daniel,
There is a synchronized (lock) { ... } syntax too.
Using a reentrant lock also offers more possibility for evolutions
like try locking or timeout locking - which you can't do with a
monitor (unless you reinvent ReentrantLock)...
Ok, fair enough.
Now that I studied the synchronization in more detail, I must say, huh,
it's very tricky. The state is composed of 3 boolean flags (that's
theoretically 8 states) that are sometimes modified in pairs under lock
and sometimes individually under lock and sometimes (shutdown hook) not
under lock. So one has to pay attention to all possible interleavings.
Anyway, I think I found a bug (not synchronization related, but
re-entrancy related). In initializeGlobalHandlers() In line 1629 you
check if initializeGlobalHandlersCalled is true and return (a recursive
call), but you do this inside the try block and therefore finally block
is executed which sets initializeGlobalHandlersDone to true. This is
prematurely, as loadLoggerHandlers might not be finished yet.
I think all possible states can be compressed into just 4 and this can
be an int so that it is always updated atomically (easier to reason
about). This way you can also get initial state to be default value (0).
Here's my attempt at simplifying this:
http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.01/
So what do you think?
Regards, Peter
best regards,
-- daniel
Regards, Peter
