Looking for feedback LOG4J2-819

Discussion:

Gary Gregory

2014-09-10 14:09:41 UTC

Hi All:

I am looking for feedback on my patch in
https://issues.apache.org/jira/browse/LOG4J2-819

Thank you ,
Gary

--
E-Mail: ***@gmail.com | ***@apache.org
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Remko Popma

2014-09-10 23:30:10 UTC

Permalink

Gary,
Thanks for looking at this!

I had a look at the patch. Some feedback:
* Good idea to have a Clock.stop() method
* As you indicated, it may be a good idea to call
ClockFactory.getClock().stop() in the loggerContext stop() method. That way
non-webapps can benefit too.
* We may need a Clock.start() method in loggerContext.start() then when a
webapp is reloaded. - but newContext.start() may get called before
oldContext.stop()! Need a refCount mechanism??
* An alternative to creating a new class StopFlagThread is to call
thread.interrupt() in the stop() method, and check if interrupted() returns
true in the while loop: while (!interrupted()). But creating a new class
works just as well.

Tiny detail: there is a log file log4j-core/LOG4J2-807.log in the patch.

Post by Gary Gregory
I am looking for feedback on my patch in
https://issues.apache.org/jira/browse/LOG4J2-819
Thank you ,
Gary
--
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Matt Sicker

2014-09-10 23:36:09 UTC

Permalink

Should Clock extend LifeCycle?

Post by Remko Popma
Gary,
Thanks for looking at this!
* Good idea to have a Clock.stop() method
* As you indicated, it may be a good idea to call
ClockFactory.getClock().stop() in the loggerContext stop() method. That
way non-webapps can benefit too.
* We may need a Clock.start() method in loggerContext.start() then when a
webapp is reloaded. - but newContext.start() may get called before
oldContext.stop()! Need a refCount mechanism??
* An alternative to creating a new class StopFlagThread is to call
thread.interrupt() in the stop() method, and check if interrupted() returns
true in the while loop: while (!interrupted()). But creating a new class
works just as well.
Tiny detail: there is a log file log4j-core/LOG4J2-807.log in the patch.

--
Matt Sicker <***@gmail.com>

Remko Popma

2014-09-10 23:46:17 UTC

Permalink

I've thought about it some more and now I'm leaning towards not fixing
this. Starting and stopping this thread brings too much complexity; it's
just not worth it. These clocks were designed to be used in ultralow (sub-
microsecond) latency applications.
Web apps don't fall into that category.
Web apps should just use the default system clock. Using any other class
gives them no benefits and only trouble.
I will update the site docs to that effect.

Post by Matt Sicker
Should Clock extend LifeCycle?

Gary Gregory

2014-09-11 01:03:14 UTC

Permalink

Hi Remko & Matt,

Thank you for chiming in.

As of now, the threads still exist and are not cleanly stoppable. The only
way to cleanly and _simply_ stop a thread is with a flag as the patch does.
From a design stand point it does not smell right to start threads and not
offer a clean way to stop them. So that's one point I see in favor of _at
least_ having the new cleaner clocks in master. Whether or not we stop them
automatically is another issue.

I see your point about web apps but that only means (to me) that out web
integration needs to override the clock factory with one that does not use
a thread. If the user configures a thread based clock anyway, then what? He
or she is hosed? That does not sound right.

So from a process POV, I would like to:

(1) Agree on how to stop a thread-based clock (my patch minus the call from
the web code).
(2) Discuss on who should do the stopping, if any one. At worse, the use
can call the stop() method. At least we would have something in 2.1...

Thoughts?

Gary

Post by Remko Popma
I've thought about it some more and now I'm leaning towards not fixing
this. Starting and stopping this thread brings too much complexity; it's
just not worth it. These clocks were designed to be used in ultralow (sub-
microsecond) latency applications.
Web apps don't fall into that category.
Web apps should just use the default system clock. Using any other class
gives them no benefits and only trouble.
I will update the site docs to that effect.

Post by Matt Sicker
Should Clock extend LifeCycle?

Post by Remko Popma
Gary,
Thanks for looking at this!
* Good idea to have a Clock.stop() method
* As you indicated, it may be a good idea to call
ClockFactory.getClock().stop() in the loggerContext stop() method. That
way non-webapps can benefit too.
* We may need a Clock.start() method in loggerContext.start() then when
a webapp is reloaded. - but newContext.start() may get called before
oldContext.stop()! Need a refCount mechanism??
* An alternative to creating a new class StopFlagThread is to call
thread.interrupt() in the stop() method, and check if interrupted() returns
true in the while loop: while (!interrupted()). But creating a new class
works just as well.
Tiny detail: there is a log file log4j-core/LOG4J2-807.log in the patch.

Matt Sicker

2014-09-18 23:24:40 UTC

Permalink

I'm thinking each LoggerContext can contain a sort of daemon thread for our
various daemon threads that may be spawned. Use of the ThreadFactory
interface like in the async package would be a better idea than calling new
Thread(). More importantly, it would be useful to have an ExecutorService
to keep track of spawned threads. Since all our threads that get spawned
aren't really meant to be in a thread pool, we can probably use
Executors.newCachedThreadPool() or just implement AbstractExecutorService.

Post by Gary Gregory
Hi Remko & Matt,
Thank you for chiming in.
As of now, the threads still exist and are not cleanly stoppable. The only
way to cleanly and _simply_ stop a thread is with a flag as the patch does.
From a design stand point it does not smell right to start threads and not
offer a clean way to stop them. So that's one point I see in favor of _at
least_ having the new cleaner clocks in master. Whether or not we stop them
automatically is another issue.
I see your point about web apps but that only means (to me) that out web
integration needs to override the clock factory with one that does not use
a thread. If the user configures a thread based clock anyway, then what? He
or she is hosed? That does not sound right.
(1) Agree on how to stop a thread-based clock (my patch minus the call
from the web code).
(2) Discuss on who should do the stopping, if any one. At worse, the use
can call the stop() method. At least we would have something in 2.1...
Thoughts?
Gary

Post by Matt Sicker
Should Clock extend LifeCycle?

Post by Remko Popma
Gary,
Thanks for looking at this!
* Good idea to have a Clock.stop() method
* As you indicated, it may be a good idea to call
ClockFactory.getClock().stop() in the loggerContext stop() method.
That way non-webapps can benefit too.
* We may need a Clock.start() method in loggerContext.start() then when
a webapp is reloaded. - but newContext.start() may get called before
oldContext.stop()! Need a refCount mechanism??
* An alternative to creating a new class StopFlagThread is to call
thread.interrupt() in the stop() method, and check if interrupted() returns
true in the while loop: while (!interrupted()). But creating a new class
works just as well.
Tiny detail: there is a log file log4j-core/LOG4J2-807.log in the patch.

--
Java Persistence with Hibernate, Second Edition
<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

--
Matt Sicker <***@gmail.com>