Discussion:
[jira] [Created] (LOG4J2-859) Use an AtomicReference for AbstractLifeCycle to make a finite state machine
Matt Sicker (JIRA)
2014-09-25 02:13:33 UTC
Permalink
Matt Sicker created LOG4J2-859:
----------------------------------

Summary: Use an AtomicReference for AbstractLifeCycle to make a finite state machine
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker


I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Remko Popma (JIRA)
2014-09-25 02:36:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147275#comment-14147275 ]

Remko Popma commented on LOG4J2-859:
------------------------------------

Can you elaborate a bit on the motivation for this change? What problem does this solve? (I was not aware of any multi-threading issues in the life cycle...)
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Matt Sicker (JIRA)
2014-09-25 02:40:33 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147278#comment-14147278 ]

Matt Sicker commented on LOG4J2-859:
------------------------------------

No need for a volatile field, for one. Built in support for enforcing a state machine contract (e.g., you can run start() over and over again and it only starts once). Plus the error handling is rather convenient.
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Gary Gregory (JIRA)
2014-09-25 02:45:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147283#comment-14147283 ]

Gary Gregory commented on LOG4J2-859:
-------------------------------------

Matt,

Can you attach the code we are talking about here please?

It sounds like you are talking about implementation AND semantic differences all rolled up into one. We should talk about each individually.

But we can wait for after 2.1 and focus on 2.1 now ;-)

Thank you,
Gary
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Matt Sicker (JIRA)
2014-09-25 02:53:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147291#comment-14147291 ]

Matt Sicker commented on LOG4J2-859:
------------------------------------

{code}
package org.apache.logging.log4j.core;

import java.io.Serializable;
import java.util.concurrent.atomic.AtomicReference;

import org.apache.logging.log4j.core.util.Throwables;
import org.apache.logging.log4j.status.StatusLogger;

/**
* An extensible {@link LifeCycle} using an {@link AtomicReference} to wrap its {@link LifeCycle.State}. Thus, classes
* which extend this class will follow the finite state machine as follows:
* <ol>
* <li>When {@link #start()} is called, {@link #doStart()} is called if and only if this is in the INITIALIZED state or
* the STOPPED state.</li>
* <li>Before {@link #doStart()} is called, this will be in the STARTING state.</li>
* <li>After {@link #doStart()} is called, this will be in the STARTED state if no exception was thrown; otherwise,
* this will be in the INITIALIZED state again, and the exception thrown will be re-thrown (if unchecked) or wrapped
* in an {@link java.lang.reflect.UndeclaredThrowableException} and then rethrown (if checked).</li>
* <li>When {@link #stop()} is called, {@link #doStop()} is called if and only if this is in the STARTED state.</li>
* <li>Before {@link #doStop()} is called, this will be in the STOPPING state.</li>
* <li>After {@link #doStop()} is called, this will be in the STOPPED state. Any exceptions thrown will be re-thrown
* as described above.</li>
* </ol>
*/
public abstract class AbstractAtomicLifeCycle implements LifeCycle, Serializable {

private static final long serialVersionUID = 1L;

protected static final StatusLogger LOGGER = StatusLogger.getLogger();

private final AtomicReference<State> state = new AtomicReference<State>(State.INITIALIZED);

@Override
public void start() {
if (state.compareAndSet(State.INITIALIZED, State.STARTING) ||
state.compareAndSet(State.STOPPED, State.STARTING)) {
try {
doStart();
state.set(State.STARTED);
} catch (final Exception e) {
state.set(State.INITIALIZED);
Throwables.rethrow(e);
}
}
}

/**
* Performs the start-up logic. This method is called only if this is in the INITIALIZED or STOPPED state.
*
* @throws Exception
*/
protected abstract void doStart() throws Exception;

@Override
public void stop() {
if (state.compareAndSet(State.STARTED, State.STOPPING)) {
try {
doStop();
} catch (Exception e) {
Throwables.rethrow(e);
} finally {
state.set(State.STOPPED);
}
}
}

/**
* Performs the tear-down logic. This method is called only if this is in the STARTED state.
*
* @throws Exception
*/
protected abstract void doStop() throws Exception;

@Override
public boolean isStarted() {
return state.get() == State.STARTED;
}

@Override
public boolean isStopped() {
return state.get() == State.STOPPED;
}

@Override
public boolean equals(final Object o) {
if (this == o) {
return true;
}
if (o == null || getClass() != o.getClass()) {
return false;
}
final AbstractAtomicLifeCycle that = (AbstractAtomicLifeCycle) o;
return state.equals(that.state);
}

@Override
public int hashCode() {
return state.hashCode();
}
}
{code}
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Matt Sicker (JIRA)
2014-09-25 14:07:33 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147759#comment-14147759 ]

Matt Sicker commented on LOG4J2-859:
------------------------------------

Here's a thread-safety scenario: AbstractAppender.setHandler(ErrorHandler) performs a check to see if {{this.isStarted()}}. Afterward, the ErrorHandler is set. However, the Appender could already be started by that point. Though this probably requires locking rather than the atomic reference, but the point remains.
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Gary Gregory (JIRA)
2014-09-25 14:53:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Gary Gregory updated LOG4J2-859:
--------------------------------
Comment: was deleted

(was: I do not see a call to {{isStarted()}} in {{org.apache.logging.log4j.core.appender.AbstractAppender.setHandler(ErrorHandler)}}:

{code:java}
/**
* The handler must be set before the appender is started.
* @param handler The ErrorHandler to use.
*/
@Override
public void setHandler(final ErrorHandler handler) {
if (handler == null) {
LOGGER.error("The handler cannot be set to null");
}
if (isStarted()) {
LOGGER.error("The handler cannot be changed once the appender is started");
return;
}
this.handler = handler;
}
{code}

?)
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Gary Gregory (JIRA)
2014-09-25 14:53:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147821#comment-14147821 ]

Gary Gregory commented on LOG4J2-859:
-------------------------------------

I do not see a call to {{isStarted()}} in {{org.apache.logging.log4j.core.appender.AbstractAppender.setHandler(ErrorHandler)}}:

{code:java}
/**
* The handler must be set before the appender is started.
* @param handler The ErrorHandler to use.
*/
@Override
public void setHandler(final ErrorHandler handler) {
if (handler == null) {
LOGGER.error("The handler cannot be set to null");
}
if (isStarted()) {
LOGGER.error("The handler cannot be changed once the appender is started");
return;
}
this.handler = handler;
}
{code}

?
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Gary Gregory (JIRA)
2014-09-25 14:58:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147827#comment-14147827 ]

Gary Gregory commented on LOG4J2-859:
-------------------------------------

Hm... there are no callers of {{org.apache.logging.log4j.core.Appender.setHandler(ErrorHandler)}}. Do we even need that method?

Let's imagine there is at least one.

Are you saying that more than once thread can call {{AbstractAppender.setHandler()}} on the same appender instance?

That could happen when? When some app starts up and sets handlers from different threads? I guess.

But how would a different LC impl change that?
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Remko Popma (JIRA)
2014-09-25 15:27:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14147859#comment-14147859 ]

Remko Popma commented on LOG4J2-859:
------------------------------------

Matt, please take this as constructive criticism, but I think you still have a tendency to dive deep into a solution before clarifying for yourself and for others what problem you are trying to solve.

"Use an AtomicReference for AbstractLifeCycle to make a finite state machine" is a solution, not a problem description.

Now that we are discussing this, the problem is now (to me) slowly taking shape as "It is easy for applications that do programmatic configuration to do things in the wrong order. Furthermore there is a risk that users try to modify a configuration that is already started (and in use)."

I think it helps to focus on a crisp problem definition first. "How to make programmatic configuration more robust" generates all kinds of interesting ideas: what kind of user mistakes are possible? Which are most likely? What options do we have for addressing this? Should we allow modifying live configurations? What about cloning a config, modifying the clone, then reconfiguring with the modified clone? Etc...

If we focus on a particular solution too soon we lose all that...
Again, I mean this in a positive way.
-Remko
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
Matt Sicker (JIRA)
2014-09-25 18:14:34 UTC
Permalink
[ https://issues.apache.org/jira/browse/LOG4J2-859?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14148065#comment-14148065 ]

Matt Sicker commented on LOG4J2-859:
------------------------------------

That is some very helpful advice, Remko. I'm so used to solving other people's user stories that I never really develop my own all that often. You're completely right in that I jump to the solution rather than the problem. :)

Yes, this has to do with programmatic configuration and ensuring everything follows standard semantics. This is helpful for diagnosing errors or bugs in custom plugins. Plus, the more convention over configuration we can provide, the easier it is to write robust plugins.

In regard to the setHandler method call, I'm not sure if it's currently possible, but a theoretical weakness is that while one thread is calling {{start()}}, another thread could be calling {{setHandler()}}. This could possibly be caused by clever workarounds in attempting programmatic configuration without properly understanding the execution context in which the user may be trying to work in. Frameworks like Spring really go all out in that configurability, but that's not really all that necessary here as far as I can tell.

Essentially, the log4j-api is well documented with really straightforward classes and APIs to work with. In log4j-core, however, to an unfamiliar developer, it may not be entirely clear how to do things the "Log4j way" so to say.
Post by Matt Sicker (JIRA)
Use an AtomicReference for AbstractLifeCycle to make a finite state machine
---------------------------------------------------------------------------
Key: LOG4J2-859
URL: https://issues.apache.org/jira/browse/LOG4J2-859
Project: Log4j 2
Issue Type: Improvement
Components: Core
Affects Versions: 2.2
Reporter: Matt Sicker
I already showed off an example of how this would be done in git commit {{1a332afa33c55a72ae8ab5ec83cd5964de3fdc67}}. This would also provide error handling and proper state-setting in the start and stop methods.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Loading...