On 04/05/15 16:46, Peter Levart wrote:
Hi Daniel,
Here it is:
http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.04/
Looks good for me Peter :-)
Hopefully Mandy will like it too!
All java/util/logging tests pass for me except
java/util/logging/TestLoggerWeakRefLeak
#section:build
----------messages:(2/93)----------
command: build jdk.testlibrary.*
reason: User specified action: run build jdk.testlibrary.*
result: Not run. Test running...
test result: Error. Can't find source file: jdk/testlibrary/*.java in
directory-list: /home/peter/work/hg/jdk9-dev/jdk/test/java/util/logging
/home/peter/work/hg/jdk9-dev/jdk/test/lib/testlibrary
I assume you may have a somewhat older version of jtreg that
doesn't support the .* pattern in @library / @build
I have imported your patch and tested locally - on my machine all
the logging tests have passed.
I have also run a test campaign against it and things looked good
there as well.
best regards,
-- daniel
Regards, Peter
On 05/04/2015 04:09 PM, Peter Levart wrote:
On 05/04/2015 03:46 PM, Daniel Fuchs wrote:
On 04/05/15 15:26, Peter Levart wrote:
Hi Daniel, Mandy,
What about the following:
http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.03/
You see, no boolean flags needed. The globalHandlersState is always
changed atomically within a locked region, so a graph of transitions
can
be drawn:
http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/LogManager.globalHandlersState.png
STATE_INITIALIZING and STATE_READING_CONFIG are intra-locked-region
states used to prevent infinite recursion in initializeGlobalHandlers()
and communicating state from within locked-region of
readConfiguration()
to nested reset(), respectively, but never from one locked region to
another.
That looks better.
I would consider removing these lines:
1333 if (globalHandlersState == STATE_SHUTDOWN) {
1334 // already in terminal state
1335 return;
1336 }
in favor of:
1354 } else if (globalHandlersState != STATE_SHUTDOWN) {
1355 // ...user calling reset()...
and let the reset reset twice if it's called from different contexts.
There may be permissions that could prevent a user-called reset() to
close all handlers, and we don't want that to prevent the Cleaner-called
reset to come afterwards and perform its job.
Then we don't need STATE_SHUTTING_DOWN. Cleaner.run() can change
directly to STATE_SHUTDOWN and reset() would just leave it there, but
perform the reset anyway.
Will create another variant including your test too.
Regards, Peter
BTW - in the end - one of us will need to push this together with
the new test :-)
cheers,
-- daniel
Regards, Peter
On 05/01/2015 02:03 AM, Peter Levart wrote:
On 04/30/2015 04:42 PM, Daniel Fuchs wrote:
On 28/04/15 17:46, Peter Levart wrote:
On 04/28/2015 04:57 PM, Daniel Fuchs wrote:
Here's my attempt at simplifying this:
http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.01/
LogManager can be subclassed, and subclasses may override
reset() for
different purposes.
So I'm afraid the Cleaner thread still needs to call te public
reset() method. The same unfortunately applies to
readConfiguration().
best regards,
-- daniel
Um, yes of course. This can be fixed:
http://cr.openjdk.java.net/~plevart/misc/LogManager.synchronization/webrev.02/
Hi Peter,
Sorry for the late reply.
My gut feeling is that I dislike multi-state ints.
But I guess that's a matter of taste - so I could probably
overcome it ;-)
Isn't that a simplification (of reasoning)? In particular if
individual boolean flags are read and/or written without holding
any lock.
What makes me less happy is that I had managed to
remove the explicit synchronized() { } block in the
Cleaner thread - and I now see it's back.
Could we maybe keep the imminentDeath boolean and remove
the explicit locking in the Cleaner thread?
I mean... imminentDeath should be checked from within
the lock - before doing anything. But it does not need
to be set from within a locked section.
best regards,
-- daniel
Hi Daniel, Mandy,
Explicit locking in Cleaner is something that is performed in reset()
anyway, so getting rid of it is not actually getting rid of it, as
Cleaner is calling reset() anyway. The lock is reentrant.
But If you want get rid of it so that reset() is not called under lock
held because reset() might be overridden, then imminentDeath must
return.
@Mandy:
In webrev.01 we had a private reset(int newState), called from
reset(), Cleaner and readConfiguration(), but Daniel then spotted that
reset() is an overridable method, so it has to be called from Cleaner
and readConfiguration() too, unfortunately.
The STATE_XXX names are best depicted if looking at code in
initializeGlobalHandlers() method.
Let me prepare a changed webrew...
Regards, Peter
