You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@myfaces.apache.org by Simon Kitching <sk...@apache.org> on 2006/02/19 02:51:47 UTC
Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
On Sun, 2006-02-19 at 00:46 +0000, mmarinschek@apache.org wrote:
> Author: mmarinschek
> Date: Sat Feb 18 16:46:18 2006
> New Revision: 378805
>
> URL: http://svn.apache.org/viewcvs?rev=378805&view=rev
> Log:
> minor changes in application-factory, fixed "readOnly" referral in HtmlRendererUtils
>
> Modified:
> myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
>
> Modified: myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
> URL: http://svn.apache.org/viewcvs/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java?rev=378805&r1=378804&r2=378805&view=diff
> ==============================================================================
> --- myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java (original)
> +++ myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java Sat Feb 18 16:46:18 2006
> @@ -29,12 +29,14 @@
> public class ApplicationFactoryImpl
> extends ApplicationFactory
> {
> - private static final Log log = LogFactory.getLog(ApplicationFactory.class);
> + private static final Log log = LogFactory.getLog(ApplicationFactoryImpl.class);
Just a warning to all developers: when using commons-logging in a
library, STATIC fields must **NOT** be used to store Log objects.
The problem is that the class may be called with various thread-context
classloaders (TCCL) set. However the class is only ever loaded once.
If the Log object is static, then it is initialised using the TCCL of
the very first webapp to use it (ie which forces the class to be loaded)
- including picking up config info from that TCCL. All other webapps
will then use the same Log object - which is not healthy.
Yes this is a nuisance, but it's absolutely necessary to avoid storing
Log instances in static fields for libraries like MyFaces.
Regards,
Simon
Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Simon Kitching <sk...@apache.org>.
On Sun, 2006-02-19 at 11:36 +0100, Martin Marinschek wrote:
> ok - so that means we'll need to drop static?
Yes. In fact, because MyFaces libs are often placed in a "shared"
classpath, static should be avoided in almost all cases I expect.
>
> won't that cause performance problems?
Calling LogFactory.getLog is *reasonably* efficient.
The impact is trivial for long-lived objects like
ApplicationFactoryImpl. I wouldn't worry about performance for things
like UIComponent objects either, which have a lifetime of a request.
If there are very short-lived objects which are frequently created, then
it might be worth considering either:
(a) only fetching a Log object if logging is actually needed, ie
if (some condition) {
Log log = LogFactory.getLog(Foo.class);
log.warn(....);
}
(b) using the Log object stored on some other component.
longLivedObject.getXYZLog().debug(....)
Of course this works only when there is some suitable
longer-lived object that is easily accessable from the
short-lived object that wants to do logging.
Cheers,
Simon
>
> regards,
>
> Martin
>
> On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > On Sun, 2006-02-19 at 00:46 +0000, mmarinschek@apache.org wrote:
> > > Author: mmarinschek
> > > Date: Sat Feb 18 16:46:18 2006
> > > New Revision: 378805
> > >
> > > URL: http://svn.apache.org/viewcvs?rev=378805&view=rev
> > > Log:
> > > minor changes in application-factory, fixed "readOnly" referral in HtmlRendererUtils
> > >
> > > Modified:
> > > myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
> > >
> > > Modified: myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
> > > URL: http://svn.apache.org/viewcvs/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java?rev=378805&r1=378804&r2=378805&view=diff
> > > ==============================================================================
> > > --- myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java (original)
> > > +++ myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java Sat Feb 18 16:46:18 2006
> > > @@ -29,12 +29,14 @@
> > > public class ApplicationFactoryImpl
> > > extends ApplicationFactory
> > > {
> > > - private static final Log log = LogFactory.getLog(ApplicationFactory.class);
> > > + private static final Log log = LogFactory.getLog(ApplicationFactoryImpl.class);
> >
> >
> > Just a warning to all developers: when using commons-logging in a
> > library, STATIC fields must **NOT** be used to store Log objects.
> >
> > The problem is that the class may be called with various thread-context
> > classloaders (TCCL) set. However the class is only ever loaded once.
> >
> > If the Log object is static, then it is initialised using the TCCL of
> > the very first webapp to use it (ie which forces the class to be loaded)
> > - including picking up config info from that TCCL. All other webapps
> > will then use the same Log object - which is not healthy.
> >
> > Yes this is a nuisance, but it's absolutely necessary to avoid storing
> > Log instances in static fields for libraries like MyFaces.
> >
> > Regards,
> >
> > Simon
> >
> >
>
>
> --
>
> http://www.irian.at
>
> Your JSF powerhouse -
> JSF Consulting, Development and
> Courses in English and German
>
> Professional Support for Apache MyFaces
Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Martin Marinschek <ma...@gmail.com>.
ok - so that means we'll need to drop static?
won't that cause performance problems?
regards,
Martin
On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> On Sun, 2006-02-19 at 00:46 +0000, mmarinschek@apache.org wrote:
> > Author: mmarinschek
> > Date: Sat Feb 18 16:46:18 2006
> > New Revision: 378805
> >
> > URL: http://svn.apache.org/viewcvs?rev=378805&view=rev
> > Log:
> > minor changes in application-factory, fixed "readOnly" referral in HtmlRendererUtils
> >
> > Modified:
> > myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
> >
> > Modified: myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
> > URL: http://svn.apache.org/viewcvs/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java?rev=378805&r1=378804&r2=378805&view=diff
> > ==============================================================================
> > --- myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java (original)
> > +++ myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java Sat Feb 18 16:46:18 2006
> > @@ -29,12 +29,14 @@
> > public class ApplicationFactoryImpl
> > extends ApplicationFactory
> > {
> > - private static final Log log = LogFactory.getLog(ApplicationFactory.class);
> > + private static final Log log = LogFactory.getLog(ApplicationFactoryImpl.class);
>
>
> Just a warning to all developers: when using commons-logging in a
> library, STATIC fields must **NOT** be used to store Log objects.
>
> The problem is that the class may be called with various thread-context
> classloaders (TCCL) set. However the class is only ever loaded once.
>
> If the Log object is static, then it is initialised using the TCCL of
> the very first webapp to use it (ie which forces the class to be loaded)
> - including picking up config info from that TCCL. All other webapps
> will then use the same Log object - which is not healthy.
>
> Yes this is a nuisance, but it's absolutely necessary to avoid storing
> Log instances in static fields for libraries like MyFaces.
>
> Regards,
>
> Simon
>
>
--
http://www.irian.at
Your JSF powerhouse -
JSF Consulting, Development and
Courses in English and German
Professional Support for Apache MyFaces
Re: logging (was Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Martin Marinschek <ma...@gmail.com>.
Hmm... didn't we settle on JDK 1.4 a while ago?
Simon has some other arguments on not using JDK logging, see above.
regards,
Martin
On 2/20/06, Craig McClanahan <cr...@apache.org> wrote:
>
>
> On 2/19/06, Martin Marinschek <ma...@gmail.com> wrote:
> > Indeed.
> >
> > this really means I'd be willing to switch to JDK logging. What was
> > the reason again we couldn't do that?
>
> If we are willing to live with a "JDK 1.4 or later" restriction, no reason
> at all. That, however, would seem to be an issue for some current users.
>
> > regards,
> >
> > Martin
>
> Craig
>
> > On 2/20/06, Adam Winer <aw...@gmail.com> wrote:
> > > Weeellll.... if you implement StateHolder, this isn't an issue.
> > > The public no-arg constructor will be used, variable initializer
> > > expressions will run, etc.
> > >
> > > If you implement Serializable instead, then Craig's totally right -
> > > transient variables will not be re-initialized. You can deal with
> > > this by adding the log() method, or by providing an implementation
> > > of:
> > >
> > > private void readObject(ObjectInputStream in)
> > >
> > > ... which can re-initialize any transient values.
> > >
> > > I am *so* thankful that java.util.logging doesn't force any of
> > > this pain on its users.
> > >
> > > -- Adam
> > >
> > >
> > >
> > > On 2/19/06, Craig McClanahan <cr...@apache.org> wrote:
> > > >
> > > >
> > > >
> > > > On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > > > > On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
> > > > >
> > > > > > Simon,
> > > > > >
> > > > > > Could you do me a favor and publicize this in the Struts community
> as
> > > > > > well? The framework code there is littered with static log
> instances
> > > > > > to.
> > > > >
> > > > > Will do.
> > > > >
> > > > > > You might also want to add some notes related to using Log
> instances
> > > > > > in Serializable classes (see below).
> > > > >
> > > > > Will do that also.
> > > > >
> > > > > >
> > > > > > MyFaces folks,
> > > > > >
> > > > > > There *is* a JSF-specific consideration to think about, if you
> have
> > > > > > classes that implement StateHolder (like a UIComponent
> > > > > > implementation). Log instances will generally *not* be
> serializable,
> > > > > > so you will need to deal specially with them in saveState() and
> > > > > > restoreState() methods. The simplest thing is to just not save
> them,
> > > > > > and do a "new" operation again in restoreState().
> > > > >
> > > > > Sorry but I don't understand. Why won't the normal approach work?
> > > > >
> > > > > public class SomeComponent .... {
> > > > > private Log log = LogFactory.getLog(SomeComponent.class);
> > > > > ...
> > > > > }
> > > > >
> > > > > AIUI, the log object won't be saved in the saveState method, but it
> will
> > > > > be recreated nicely during the RestoreView phase when a new instance
> is
> > > > > created for the state to be restored into.
> > > > >
> > > > > >
> > > > > > Along the same lines, if your class implements Serializable, you
> will
> > > > > > need to mark the instance variable transient. I've started using
> the
> > > > > > following pattern in my Serializable classes, which would work
> inside
> > > > > > a StateHolder as well:
> > > > > >
> > > > > > private transient Log log = null;
> > > > > >
> > > > > > private Log log() {
> > > > > > if (log == null) {
> > > > > > log = LogFactory.getLog(...);
> > > > > > }
> > > > > > return log;
> > > > > > }
> > > > > >
> > > > > > and a typical call looks like:
> > > > > >
> > > > > > if (log().isDebugEnabled()) {
> > > > > > log().debug("...");
> > > > > > }
> > > > >
> > > > > Ok, transient is needed here. But apart from that why won't the
> standard
> > > > > approach work?
> > > > >
> > > > > public class SomeThing implements Serializable {
> > > > > private transient Log log = LogFactory.getLog (SomeThing.class);
> > > > > ...
> > > > > if ( log.isDebugEnabled()) {
> > > > > log.debug("...");
> > > > > }
> > > > > }
> > > > >
> > > > > Doesn't the log object get recreated when the deserialization
> occurs?
> > > >
> > > > No, AIUI. When an object is deserialized, it does *not* execute the
> > > > variable initializer expressions. Since it was declared transient,
> there's
> > > > no state for that object to be restored.
> > > >
> > > > > The log() method is quite a lot of boilerplate, and also imposes an
> > > > > additional method call for every isDebugEnabled() call. [The extra
> call
> > > > > inside the guard is not really relevant as output *is* going to
> occur at
> > > > > this point which greatly outweighs the price of the method call].
> > > >
> > > > You can reduce that by one call by protecting only the conditional
> > > > expression, because if you get inside you know there's an object ...
> but
> > > > that means you are relying on the implicit contract between the log
> instance
> > > > variable and the log() method.
> > > >
> > > > > Cheers,
> > > > >
> > > > > Simon
> > > > >
> > > > >
> > > > Craig
> > > >
> > > >
> > >
> >
> >
> > --
> >
> > http://www.irian.at
> >
> > Your JSF powerhouse -
> > JSF Consulting, Development and
> > Courses in English and German
> >
> > Professional Support for Apache MyFaces
> >
>
>
--
http://www.irian.at
Your JSF powerhouse -
JSF Consulting, Development and
Courses in English and German
Professional Support for Apache MyFaces
Re: logging (was Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Craig McClanahan <cr...@apache.org>.
On 2/19/06, Martin Marinschek <ma...@gmail.com> wrote:
>
> Indeed.
>
> this really means I'd be willing to switch to JDK logging. What was
> the reason again we couldn't do that?
If we are willing to live with a "JDK 1.4 or later" restriction, no reason
at all. That, however, would seem to be an issue for some current users.
regards,
>
> Martin
Craig
On 2/20/06, Adam Winer <aw...@gmail.com> wrote:
> > Weeellll.... if you implement StateHolder, this isn't an issue.
> > The public no-arg constructor will be used, variable initializer
> > expressions will run, etc.
> >
> > If you implement Serializable instead, then Craig's totally right -
> > transient variables will not be re-initialized. You can deal with
> > this by adding the log() method, or by providing an implementation
> > of:
> >
> > private void readObject(ObjectInputStream in)
> >
> > ... which can re-initialize any transient values.
> >
> > I am *so* thankful that java.util.logging doesn't force any of
> > this pain on its users.
> >
> > -- Adam
> >
> >
> >
> > On 2/19/06, Craig McClanahan <cr...@apache.org> wrote:
> > >
> > >
> > >
> > > On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > > > On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
> > > >
> > > > > Simon,
> > > > >
> > > > > Could you do me a favor and publicize this in the Struts community
> as
> > > > > well? The framework code there is littered with static log
> instances
> > > > > to.
> > > >
> > > > Will do.
> > > >
> > > > > You might also want to add some notes related to using Log
> instances
> > > > > in Serializable classes (see below).
> > > >
> > > > Will do that also.
> > > >
> > > > >
> > > > > MyFaces folks,
> > > > >
> > > > > There *is* a JSF-specific consideration to think about, if you
> have
> > > > > classes that implement StateHolder (like a UIComponent
> > > > > implementation). Log instances will generally *not* be
> serializable,
> > > > > so you will need to deal specially with them in saveState() and
> > > > > restoreState() methods. The simplest thing is to just not save
> them,
> > > > > and do a "new" operation again in restoreState().
> > > >
> > > > Sorry but I don't understand. Why won't the normal approach work?
> > > >
> > > > public class SomeComponent .... {
> > > > private Log log = LogFactory.getLog(SomeComponent.class);
> > > > ...
> > > > }
> > > >
> > > > AIUI, the log object won't be saved in the saveState method, but it
> will
> > > > be recreated nicely during the RestoreView phase when a new instance
> is
> > > > created for the state to be restored into.
> > > >
> > > > >
> > > > > Along the same lines, if your class implements Serializable, you
> will
> > > > > need to mark the instance variable transient. I've started using
> the
> > > > > following pattern in my Serializable classes, which would work
> inside
> > > > > a StateHolder as well:
> > > > >
> > > > > private transient Log log = null;
> > > > >
> > > > > private Log log() {
> > > > > if (log == null) {
> > > > > log = LogFactory.getLog(...);
> > > > > }
> > > > > return log;
> > > > > }
> > > > >
> > > > > and a typical call looks like:
> > > > >
> > > > > if (log().isDebugEnabled()) {
> > > > > log().debug("...");
> > > > > }
> > > >
> > > > Ok, transient is needed here. But apart from that why won't the
> standard
> > > > approach work?
> > > >
> > > > public class SomeThing implements Serializable {
> > > > private transient Log log = LogFactory.getLog (SomeThing.class);
> > > > ...
> > > > if (log.isDebugEnabled()) {
> > > > log.debug("...");
> > > > }
> > > > }
> > > >
> > > > Doesn't the log object get recreated when the deserialization
> occurs?
> > >
> > > No, AIUI. When an object is deserialized, it does *not* execute the
> > > variable initializer expressions. Since it was declared transient,
> there's
> > > no state for that object to be restored.
> > >
> > > > The log() method is quite a lot of boilerplate, and also imposes an
> > > > additional method call for every isDebugEnabled() call. [The extra
> call
> > > > inside the guard is not really relevant as output *is* going to
> occur at
> > > > this point which greatly outweighs the price of the method call].
> > >
> > > You can reduce that by one call by protecting only the conditional
> > > expression, because if you get inside you know there's an object ...
> but
> > > that means you are relying on the implicit contract between the log
> instance
> > > variable and the log() method.
> > >
> > > > Cheers,
> > > >
> > > > Simon
> > > >
> > > >
> > > Craig
> > >
> > >
> >
>
>
> --
>
> http://www.irian.at
>
> Your JSF powerhouse -
> JSF Consulting, Development and
> Courses in English and German
>
> Professional Support for Apache MyFaces
>
Re: logging (was Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Simon Kitching <sk...@apache.org>.
On Mon, 2006-02-20 at 07:59 +0100, Martin Marinschek wrote:
> Indeed.
>
> this really means I'd be willing to switch to JDK logging. What was
> the reason again we couldn't do that?
The JSF 1.1 spec requires java 1.3 compatibility. At least that's what
this email stated:
http://marc.theaimsgroup.com/?l=myfaces-dev&m=113398269804780&w=2
Cheers,
Simon
Re: logging (was Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Martin Marinschek <ma...@gmail.com>.
Indeed.
this really means I'd be willing to switch to JDK logging. What was
the reason again we couldn't do that?
regards,
Martin
On 2/20/06, Adam Winer <aw...@gmail.com> wrote:
> Weeellll.... if you implement StateHolder, this isn't an issue.
> The public no-arg constructor will be used, variable initializer
> expressions will run, etc.
>
> If you implement Serializable instead, then Craig's totally right -
> transient variables will not be re-initialized. You can deal with
> this by adding the log() method, or by providing an implementation
> of:
>
> private void readObject(ObjectInputStream in)
>
> ... which can re-initialize any transient values.
>
> I am *so* thankful that java.util.logging doesn't force any of
> this pain on its users.
>
> -- Adam
>
>
>
> On 2/19/06, Craig McClanahan <cr...@apache.org> wrote:
> >
> >
> >
> > On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > > On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
> > >
> > > > Simon,
> > > >
> > > > Could you do me a favor and publicize this in the Struts community as
> > > > well? The framework code there is littered with static log instances
> > > > to.
> > >
> > > Will do.
> > >
> > > > You might also want to add some notes related to using Log instances
> > > > in Serializable classes (see below).
> > >
> > > Will do that also.
> > >
> > > >
> > > > MyFaces folks,
> > > >
> > > > There *is* a JSF-specific consideration to think about, if you have
> > > > classes that implement StateHolder (like a UIComponent
> > > > implementation). Log instances will generally *not* be serializable,
> > > > so you will need to deal specially with them in saveState() and
> > > > restoreState() methods. The simplest thing is to just not save them,
> > > > and do a "new" operation again in restoreState().
> > >
> > > Sorry but I don't understand. Why won't the normal approach work?
> > >
> > > public class SomeComponent .... {
> > > private Log log = LogFactory.getLog(SomeComponent.class);
> > > ...
> > > }
> > >
> > > AIUI, the log object won't be saved in the saveState method, but it will
> > > be recreated nicely during the RestoreView phase when a new instance is
> > > created for the state to be restored into.
> > >
> > > >
> > > > Along the same lines, if your class implements Serializable, you will
> > > > need to mark the instance variable transient. I've started using the
> > > > following pattern in my Serializable classes, which would work inside
> > > > a StateHolder as well:
> > > >
> > > > private transient Log log = null;
> > > >
> > > > private Log log() {
> > > > if (log == null) {
> > > > log = LogFactory.getLog(...);
> > > > }
> > > > return log;
> > > > }
> > > >
> > > > and a typical call looks like:
> > > >
> > > > if (log().isDebugEnabled()) {
> > > > log().debug("...");
> > > > }
> > >
> > > Ok, transient is needed here. But apart from that why won't the standard
> > > approach work?
> > >
> > > public class SomeThing implements Serializable {
> > > private transient Log log = LogFactory.getLog (SomeThing.class);
> > > ...
> > > if (log.isDebugEnabled()) {
> > > log.debug("...");
> > > }
> > > }
> > >
> > > Doesn't the log object get recreated when the deserialization occurs?
> >
> > No, AIUI. When an object is deserialized, it does *not* execute the
> > variable initializer expressions. Since it was declared transient, there's
> > no state for that object to be restored.
> >
> > > The log() method is quite a lot of boilerplate, and also imposes an
> > > additional method call for every isDebugEnabled() call. [The extra call
> > > inside the guard is not really relevant as output *is* going to occur at
> > > this point which greatly outweighs the price of the method call].
> >
> > You can reduce that by one call by protecting only the conditional
> > expression, because if you get inside you know there's an object ... but
> > that means you are relying on the implicit contract between the log instance
> > variable and the log() method.
> >
> > > Cheers,
> > >
> > > Simon
> > >
> > >
> > Craig
> >
> >
>
--
http://www.irian.at
Your JSF powerhouse -
JSF Consulting, Development and
Courses in English and German
Professional Support for Apache MyFaces
Re: logging (was Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Adam Winer <aw...@gmail.com>.
On 2/20/06, Simon Kitching <sk...@apache.org> wrote:
> On Mon, 2006-02-20 at 11:51 -0800, Adam Winer wrote:
> > IIRC, you have a personal stake in this issue, but the
> > plain truth is that there is no war anymore - java.util.logging
> > won by Sun's fiat. Technical superiority is not the ultimate
> > arbiter.
> >
>
> Yes, I am a commons-logging committer. That doesn't mean I believe it's
> perfect; I work on it because I need it, and because it *does* have
> flaws that need work.
>
> I don't have any interest in defending JCL against true complaints; the
> ClassCastException problems experienced with 1.0.4 and earlier are an
> example (though one that is at least partially caused by what I consider
> to be bad container design in many cases).
>
> The recent problems pointed out about Log not being Serializable and the
> implications thereof are another; that's truly ugly. However it appears
> that no-one has ever bothered to point this out on the email lists or
> bugzilla. I've reported this to the right lists, and there may well be a
> fix in the upcoming release.
>
> However there is a lot of truly incorrect information floating around
> about JCL; one person makes a false claim, another quotes it, then it
> becomes accepted as "truth" despite the fact that it is complete
> garbage. Unfortunately a number of articles linked to from the log4j
> website fall into this category. I do try to correct such claims when I
> see them because having false info does no-one any favours.
>
> BTW, I also agree that j.u.logging will eventually be *the* logging API,
> and I'll be glad when that day comes and JCL can be retired. Logging
> definitely should be in the language core. It's a shame Sun failed to
> consult more widely with the community on design but nevertheless j.u.l
> is *useable*. However there's a whole lot of people on pre-1.4 JVMs, and
> that's going to continue to be the case for a number of years.
>
> >
> > > > I am *so* thankful that java.util.logging doesn't force any of
> > > > this pain on its users.
> > >
> > > Well, it does this by not providing the ability for EITHER of:
> > > (a) logging implementations in webapps, or
> > > (b) logging configurations in webapps
> >
> > Not exactly true: it's not enabled out-of-the-box (which I agree
> > is lousy), but a solid JDK 1.4 application server can enable
> > it. I know OC4J does.
> >
> > > The moment a non-standard java.util.logging implementation tries to do
> > > either, exactly this problem reappears. And then the problem is not
> > > documented in the API the user is using!
> >
> > The problem only reappears if someone other than the
> > application server tries to provide the feature. If the app
> > server provides it, it is fully in control of the classloaders
> > used, and can ensure that the Logger classes in use
> > are loaded by the parent classloader. (The fact that there
> > is one and only one LogManager necessarily puts the
> > onus on the app server.)
>
> I would *really* be interested in more info on how OC4J can do this. It
> sure seems impossible to me.
Hrm - I think I have to eat some crow here; Just checked,
and OC4J *might* be able to do it, but the included
configuration webapp only allows configuration at a global
level, not per-webapp. So it almost certainly can't.
>
> The static field on the shared class points to exactly ONE Log object,
> yes? And that Log object must have been configured from exactly ONE of:
> (a) config info in webapp A
> (b) config info in webapp B
> (c) config info in the container
>
> So how is it possible for calls from webappA and webappB to both behave
> correctly if they are accessing a shared class with a static Log member?
Shared class or not, it can't, not without proxying or additional
lookup effort.
> NB: The Log object could actually be a "proxy" where each call to each
> method looks up the TCCL then dispatches to an appropriate *real* Log
> object, but that has horrible performance implications.
Indeed, it would; the absolute best I could imagine is that
the container configures the Log to have the most verbose
level requested by any webapp, then have a Filter
check the TCCL and trim out ones that don't apply to
the current webapp. This *might* be acceptable, but
I can easily imagine circumstances where it would not -
crank up insanely verbose levels on a test app while
leaving up the real app that's getting normal test loads.
You'd be churning out LogRecords, though at least the
log files wouldn't get flooded.
> Note that we are *not* talking about classloaders at all here, except
> that the TCCL is used as a "key" for different logging configurations.
> ClassCastException issues are a different problem.
Yep. TCCL; alternatively, stash the key on a ThreadLocal,
which is probably faster still. We use ThreadLocals quite
a bit, and I keep waiting for them to show up in any profile
as a bottleneck - they never have yet.
> > Pressure should be applied to app servers to get this
> > important feature right. Tomcat doesn't, AFAIK, which
> > is basically just a big missing feature of Tomcat, not
> > a missing featurer of java.util.logging.
>
> If you can give a hint on how it's possible I'd be happy to work to see
> this gets into tomcat.
It would seem that hallucinations of imagined behavior
are the first step to implementing the feature. ;)
> > > It's easy to avoid bugs by not providing functionality. I can provide
> > > you with a totally bug-free web presentation tier in 30 seconds --
> > > provided all it ever does is display "hello world".
> > >
> > > And actually, it's not a bug - it's an architectural issue due to (IMO)
> > > the poor Java classloader model. If static variables weren't shared
> > > between webapps then the problem wouldn't occur. Ecch, the sheer
> > > ugliness of a "container" framework that causes or allows that kind of
> > > interaction between supposedly independent "component" applications!
> >
> > "It's not a bug" is of scant consolation to users (like myself)
> > who have to swear at their computer when it's throwing
> > ClassCastExceptions because code they don't control
> > didn't get the memo on not creating static Log instances.
> >
>
> Agreed. However the JCL 1.1 release has a solution for this; it's
> possible to disable tccl classloading completely. Of course that
> disables useful functionality (reducing things to the default
> j.u.logging level) but it definitely fixes any ClassCastExceptions.
Good to know.
> > For MyFaces at this time, it's a non-issue at least for the
> > API and IMPL core, since JSF 1.1 needs to support JDK 1.3,
> > and that makes java.util.logging a non-starter for those
> > parts of MyFaces. (And, IMO, it'd be odd for Tomahawk
> > to have a different JDK requirement than the JSF impl.)
>
> Yep.
>
> Regards,
>
> Simon
-- Adam
Re: logging (was Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Simon Kitching <sk...@apache.org>.
On Mon, 2006-02-20 at 11:51 -0800, Adam Winer wrote:
> IIRC, you have a personal stake in this issue, but the
> plain truth is that there is no war anymore - java.util.logging
> won by Sun's fiat. Technical superiority is not the ultimate
> arbiter.
>
Yes, I am a commons-logging committer. That doesn't mean I believe it's
perfect; I work on it because I need it, and because it *does* have
flaws that need work.
I don't have any interest in defending JCL against true complaints; the
ClassCastException problems experienced with 1.0.4 and earlier are an
example (though one that is at least partially caused by what I consider
to be bad container design in many cases).
The recent problems pointed out about Log not being Serializable and the
implications thereof are another; that's truly ugly. However it appears
that no-one has ever bothered to point this out on the email lists or
bugzilla. I've reported this to the right lists, and there may well be a
fix in the upcoming release.
However there is a lot of truly incorrect information floating around
about JCL; one person makes a false claim, another quotes it, then it
becomes accepted as "truth" despite the fact that it is complete
garbage. Unfortunately a number of articles linked to from the log4j
website fall into this category. I do try to correct such claims when I
see them because having false info does no-one any favours.
BTW, I also agree that j.u.logging will eventually be *the* logging API,
and I'll be glad when that day comes and JCL can be retired. Logging
definitely should be in the language core. It's a shame Sun failed to
consult more widely with the community on design but nevertheless j.u.l
is *useable*. However there's a whole lot of people on pre-1.4 JVMs, and
that's going to continue to be the case for a number of years.
>
> > > I am *so* thankful that java.util.logging doesn't force any of
> > > this pain on its users.
> >
> > Well, it does this by not providing the ability for EITHER of:
> > (a) logging implementations in webapps, or
> > (b) logging configurations in webapps
>
> Not exactly true: it's not enabled out-of-the-box (which I agree
> is lousy), but a solid JDK 1.4 application server can enable
> it. I know OC4J does.
>
> > The moment a non-standard java.util.logging implementation tries to do
> > either, exactly this problem reappears. And then the problem is not
> > documented in the API the user is using!
>
> The problem only reappears if someone other than the
> application server tries to provide the feature. If the app
> server provides it, it is fully in control of the classloaders
> used, and can ensure that the Logger classes in use
> are loaded by the parent classloader. (The fact that there
> is one and only one LogManager necessarily puts the
> onus on the app server.)
I would *really* be interested in more info on how OC4J can do this. It
sure seems impossible to me.
The static field on the shared class points to exactly ONE Log object,
yes? And that Log object must have been configured from exactly ONE of:
(a) config info in webapp A
(b) config info in webapp B
(c) config info in the container
So how is it possible for calls from webappA and webappB to both behave
correctly if they are accessing a shared class with a static Log member?
NB: The Log object could actually be a "proxy" where each call to each
method looks up the TCCL then dispatches to an appropriate *real* Log
object, but that has horrible performance implications.
Note that we are *not* talking about classloaders at all here, except
that the TCCL is used as a "key" for different logging configurations.
ClassCastException issues are a different problem.
>
> Pressure should be applied to app servers to get this
> important feature right. Tomcat doesn't, AFAIK, which
> is basically just a big missing feature of Tomcat, not
> a missing featurer of java.util.logging.
If you can give a hint on how it's possible I'd be happy to work to see
this gets into tomcat.
>
> > It's easy to avoid bugs by not providing functionality. I can provide
> > you with a totally bug-free web presentation tier in 30 seconds --
> > provided all it ever does is display "hello world".
> >
> > And actually, it's not a bug - it's an architectural issue due to (IMO)
> > the poor Java classloader model. If static variables weren't shared
> > between webapps then the problem wouldn't occur. Ecch, the sheer
> > ugliness of a "container" framework that causes or allows that kind of
> > interaction between supposedly independent "component" applications!
>
> "It's not a bug" is of scant consolation to users (like myself)
> who have to swear at their computer when it's throwing
> ClassCastExceptions because code they don't control
> didn't get the memo on not creating static Log instances.
>
Agreed. However the JCL 1.1 release has a solution for this; it's
possible to disable tccl classloading completely. Of course that
disables useful functionality (reducing things to the default
j.u.logging level) but it definitely fixes any ClassCastExceptions.
> For MyFaces at this time, it's a non-issue at least for the
> API and IMPL core, since JSF 1.1 needs to support JDK 1.3,
> and that makes java.util.logging a non-starter for those
> parts of MyFaces. (And, IMO, it'd be odd for Tomahawk
> to have a different JDK requirement than the JSF impl.)
Yep.
Regards,
Simon
Re: logging (was Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Adam Winer <aw...@gmail.com>.
On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> On Sun, 2006-02-19 at 22:33 -0800, Adam Winer wrote:
> > Weeellll.... if you implement StateHolder, this isn't an issue.
> > The public no-arg constructor will be used, variable initializer
> > expressions will run, etc.
> >
> > If you implement Serializable instead, then Craig's totally right -
> > transient variables will not be re-initialized. You can deal with
> > this by adding the log() method, or by providing an implementation
> > of:
> >
> > private void readObject(ObjectInputStream in)
> >
> > ... which can re-initialize any transient values.
>
> Thanks for the info.
>
> >
> > I am *so* thankful that java.util.logging doesn't force any of
> > this pain on its users.
>
> Well, it does this by not providing the ability for EITHER of:
> (a) logging implementations in webapps, or
> (b) logging configurations in webapps
Not exactly true: it's not enabled out-of-the-box (which I agree
is lousy), but a solid JDK 1.4 application server can enable
it. I know OC4J does.
> The moment a non-standard java.util.logging implementation tries to do
> either, exactly this problem reappears. And then the problem is not
> documented in the API the user is using!
The problem only reappears if someone other than the
application server tries to provide the feature. If the app
server provides it, it is fully in control of the classloaders
used, and can ensure that the Logger classes in use
are loaded by the parent classloader. (The fact that there
is one and only one LogManager necessarily puts the
onus on the app server.)
Pressure should be applied to app servers to get this
important feature right. Tomcat doesn't, AFAIK, which
is basically just a big missing feature of Tomcat, not
a missing featurer of java.util.logging.
> It's easy to avoid bugs by not providing functionality. I can provide
> you with a totally bug-free web presentation tier in 30 seconds --
> provided all it ever does is display "hello world".
>
> And actually, it's not a bug - it's an architectural issue due to (IMO)
> the poor Java classloader model. If static variables weren't shared
> between webapps then the problem wouldn't occur. Ecch, the sheer
> ugliness of a "container" framework that causes or allows that kind of
> interaction between supposedly independent "component" applications!
"It's not a bug" is of scant consolation to users (like myself)
who have to swear at their computer when it's throwing
ClassCastExceptions because code they don't control
didn't get the memo on not creating static Log instances.
IIRC, you have a personal stake in this issue, but the
plain truth is that there is no war anymore - java.util.logging
won by Sun's fiat. Technical superiority is not the ultimate
arbiter.
For MyFaces at this time, it's a non-issue at least for the
API and IMPL core, since JSF 1.1 needs to support JDK 1.3,
and that makes java.util.logging a non-starter for those
parts of MyFaces. (And, IMO, it'd be odd for Tomahawk
to have a different JDK requirement than the JSF impl.)
Regards,
Adam
Re: logging (was Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Simon Kitching <sk...@apache.org>.
On Sun, 2006-02-19 at 22:33 -0800, Adam Winer wrote:
> Weeellll.... if you implement StateHolder, this isn't an issue.
> The public no-arg constructor will be used, variable initializer
> expressions will run, etc.
>
> If you implement Serializable instead, then Craig's totally right -
> transient variables will not be re-initialized. You can deal with
> this by adding the log() method, or by providing an implementation
> of:
>
> private void readObject(ObjectInputStream in)
>
> ... which can re-initialize any transient values.
Thanks for the info.
>
> I am *so* thankful that java.util.logging doesn't force any of
> this pain on its users.
Well, it does this by not providing the ability for EITHER of:
(a) logging implementations in webapps, or
(b) logging configurations in webapps
The moment a non-standard java.util.logging implementation tries to do
either, exactly this problem reappears. And then the problem is not
documented in the API the user is using!
It's easy to avoid bugs by not providing functionality. I can provide
you with a totally bug-free web presentation tier in 30 seconds --
provided all it ever does is display "hello world".
And actually, it's not a bug - it's an architectural issue due to (IMO)
the poor Java classloader model. If static variables weren't shared
between webapps then the problem wouldn't occur. Ecch, the sheer
ugliness of a "container" framework that causes or allows that kind of
interaction between supposedly independent "component" applications!
Regards,
Simon
Re: logging (was Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Adam Winer <aw...@gmail.com>.
Weeellll.... if you implement StateHolder, this isn't an issue.
The public no-arg constructor will be used, variable initializer
expressions will run, etc.
If you implement Serializable instead, then Craig's totally right -
transient variables will not be re-initialized. You can deal with
this by adding the log() method, or by providing an implementation
of:
private void readObject(ObjectInputStream in)
... which can re-initialize any transient values.
I am *so* thankful that java.util.logging doesn't force any of
this pain on its users.
-- Adam
On 2/19/06, Craig McClanahan <cr...@apache.org> wrote:
>
>
>
> On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
> >
> > > Simon,
> > >
> > > Could you do me a favor and publicize this in the Struts community as
> > > well? The framework code there is littered with static log instances
> > > to.
> >
> > Will do.
> >
> > > You might also want to add some notes related to using Log instances
> > > in Serializable classes (see below).
> >
> > Will do that also.
> >
> > >
> > > MyFaces folks,
> > >
> > > There *is* a JSF-specific consideration to think about, if you have
> > > classes that implement StateHolder (like a UIComponent
> > > implementation). Log instances will generally *not* be serializable,
> > > so you will need to deal specially with them in saveState() and
> > > restoreState() methods. The simplest thing is to just not save them,
> > > and do a "new" operation again in restoreState().
> >
> > Sorry but I don't understand. Why won't the normal approach work?
> >
> > public class SomeComponent .... {
> > private Log log = LogFactory.getLog(SomeComponent.class);
> > ...
> > }
> >
> > AIUI, the log object won't be saved in the saveState method, but it will
> > be recreated nicely during the RestoreView phase when a new instance is
> > created for the state to be restored into.
> >
> > >
> > > Along the same lines, if your class implements Serializable, you will
> > > need to mark the instance variable transient. I've started using the
> > > following pattern in my Serializable classes, which would work inside
> > > a StateHolder as well:
> > >
> > > private transient Log log = null;
> > >
> > > private Log log() {
> > > if (log == null) {
> > > log = LogFactory.getLog(...);
> > > }
> > > return log;
> > > }
> > >
> > > and a typical call looks like:
> > >
> > > if (log().isDebugEnabled()) {
> > > log().debug("...");
> > > }
> >
> > Ok, transient is needed here. But apart from that why won't the standard
> > approach work?
> >
> > public class SomeThing implements Serializable {
> > private transient Log log = LogFactory.getLog (SomeThing.class);
> > ...
> > if (log.isDebugEnabled()) {
> > log.debug("...");
> > }
> > }
> >
> > Doesn't the log object get recreated when the deserialization occurs?
>
> No, AIUI. When an object is deserialized, it does *not* execute the
> variable initializer expressions. Since it was declared transient, there's
> no state for that object to be restored.
>
> > The log() method is quite a lot of boilerplate, and also imposes an
> > additional method call for every isDebugEnabled() call. [The extra call
> > inside the guard is not really relevant as output *is* going to occur at
> > this point which greatly outweighs the price of the method call].
>
> You can reduce that by one call by protecting only the conditional
> expression, because if you get inside you know there's an object ... but
> that means you are relying on the implicit contract between the log instance
> variable and the log() method.
>
> > Cheers,
> >
> > Simon
> >
> >
> Craig
>
>
Re: logging (was Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Craig McClanahan <cr...@apache.org>.
On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
>
> On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
>
> > Simon,
> >
> > Could you do me a favor and publicize this in the Struts community as
> > well? The framework code there is littered with static log instances
> > to.
>
> Will do.
>
> > You might also want to add some notes related to using Log instances
> > in Serializable classes (see below).
>
> Will do that also.
>
> >
> > MyFaces folks,
> >
> > There *is* a JSF-specific consideration to think about, if you have
> > classes that implement StateHolder (like a UIComponent
> > implementation). Log instances will generally *not* be serializable,
> > so you will need to deal specially with them in saveState() and
> > restoreState() methods. The simplest thing is to just not save them,
> > and do a "new" operation again in restoreState().
>
> Sorry but I don't understand. Why won't the normal approach work?
>
> public class SomeComponent .... {
> private Log log = LogFactory.getLog(SomeComponent.class);
> ...
> }
>
> AIUI, the log object won't be saved in the saveState method, but it will
> be recreated nicely during the RestoreView phase when a new instance is
> created for the state to be restored into.
>
> >
> > Along the same lines, if your class implements Serializable, you will
> > need to mark the instance variable transient. I've started using the
> > following pattern in my Serializable classes, which would work inside
> > a StateHolder as well:
> >
> > private transient Log log = null;
> >
> > private Log log() {
> > if (log == null) {
> > log = LogFactory.getLog(...);
> > }
> > return log;
> > }
> >
> > and a typical call looks like:
> >
> > if (log().isDebugEnabled()) {
> > log().debug("...");
> > }
>
> Ok, transient is needed here. But apart from that why won't the standard
> approach work?
>
> public class SomeThing implements Serializable {
> private transient Log log = LogFactory.getLog(SomeThing.class);
> ...
> if (log.isDebugEnabled()) {
> log.debug("...");
> }
> }
>
> Doesn't the log object get recreated when the deserialization occurs?
No, AIUI. When an object is deserialized, it does *not* execute the
variable initializer expressions. Since it was declared transient, there's
no state for that object to be restored.
The log() method is quite a lot of boilerplate, and also imposes an
> additional method call for every isDebugEnabled() call. [The extra call
> inside the guard is not really relevant as output *is* going to occur at
> this point which greatly outweighs the price of the method call].
You can reduce that by one call by protecting only the conditional
expression, because if you get inside you know there's an object ... but
that means you are relying on the implicit contract between the log instance
variable and the log() method.
Cheers,
>
> Simon
>
> Craig
logging (was Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java)
Posted by Simon Kitching <sk...@apache.org>.
On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
> Simon,
>
> Could you do me a favor and publicize this in the Struts community as
> well? The framework code there is littered with static log instances
> to.
Will do.
> You might also want to add some notes related to using Log instances
> in Serializable classes (see below).
Will do that also.
>
> MyFaces folks,
>
> There *is* a JSF-specific consideration to think about, if you have
> classes that implement StateHolder (like a UIComponent
> implementation). Log instances will generally *not* be serializable,
> so you will need to deal specially with them in saveState() and
> restoreState() methods. The simplest thing is to just not save them,
> and do a "new" operation again in restoreState().
Sorry but I don't understand. Why won't the normal approach work?
public class SomeComponent .... {
private Log log = LogFactory.getLog(SomeComponent.class);
...
}
AIUI, the log object won't be saved in the saveState method, but it will
be recreated nicely during the RestoreView phase when a new instance is
created for the state to be restored into.
>
> Along the same lines, if your class implements Serializable, you will
> need to mark the instance variable transient. I've started using the
> following pattern in my Serializable classes, which would work inside
> a StateHolder as well:
>
> private transient Log log = null;
>
> private Log log() {
> if (log == null) {
> log = LogFactory.getLog(...);
> }
> return log;
> }
>
> and a typical call looks like:
>
> if (log().isDebugEnabled()) {
> log().debug("...");
> }
Ok, transient is needed here. But apart from that why won't the standard
approach work?
public class SomeThing implements Serializable {
private transient Log log = LogFactory.getLog(SomeThing.class);
...
if (log.isDebugEnabled()) {
log.debug("...");
}
}
Doesn't the log object get recreated when the deserialization occurs?
The log() method is quite a lot of boilerplate, and also imposes an
additional method call for every isDebugEnabled() call. [The extra call
inside the guard is not really relevant as output *is* going to occur at
this point which greatly outweighs the price of the method call].
Cheers,
Simon
Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Sean Schofield <se...@gmail.com>.
Yes +1 for 1.0.4. Thanks for explaining all of this.
On 2/21/06, Manfred Geiler <ma...@gmail.com> wrote:
> +1 for dependency on commons-logging 1.0.4
>
> BTW, Stan (Silvert), how do you solve these logging issues in JBoss?
> AFAIK, JBoss has only one central log4j configuration. However, is
> there a way to config logging per eapp or webapp? If yes, how does
> JBoss address those issues with shared classes?
>
> Thanks,
> Manfred
>
>
>
> On 2/21/06, Simon Kitching <sk...@apache.org> wrote:
> > On Mon, 2006-02-20 at 21:47 -0800, Craig McClanahan wrote:
> > > No, I was not aware of that change ... but does it actually work?
> > > Declaring something Serializable is not by itself sufficient if there
> > > are transient variables inside the implementation. (On a separate
> > > thread on commons-dev, I recommended that there be unit tests to
> > > validate this behavior: use a Log instance, serialize it, deserialize
> > > it, and use it some more).
> >
> > I believe Serializable is correctly implemented for all standard
> > adapters except possibly AvalonLog [will follow that up on commons-dev].
> > The deserialized objects correctly locate a fresh "underlying" log
> > object to forward calls to.
> >
> > Yes there need to be unit tests for this; I'll look for them and add
> > some if not already present.
> >
> > >
> > >
> > > The 1.0.4 release occurred around June 2004 so perhaps there
> > > are a few
> > > containers out there capable of running MyFaces but which are
> > > still on
> > > JCL 1.0.3. In addition there might be a few custom Log
> > > adapters around
> > > which *might* not implement Serializable.
> > >
> > > However in view of the complicated/ugly workaround for
> > > non-serializable
> > > Log implementations, it seems ok to me for MyFaces to just
> > > declare a
> > > dependency on JCL 1.0.4. If people don't comply then there is
> > > a very
> > > obvious "NotSerializableException: logAdapterClassName"
> > > message
> > > generated. The fix is then to upgrade to 1.0.4 (which is
> > > binary
> > > compatible with earlier releases) or a later release.
> > >
> > > Comments/Opinions?
> > >
> > > +1 on declaring a dependency on Commons Logging 1.0.4 ... anything
> > > prior to that is just asking for trouble. I use 1.0.4 exclusively for
> > > eveything that has a C-L dependency ... works fine, lasts a long
> > > time :-).
> >
> > So the conclusion seems to be that if/when using commons-logging in
> > MyFaces, use:
> > private Log foo = LogFactory.getLog("foo");
> > everywhere except in short-lived and frequently-created objects. In
> > those cases, either call LogFactory.getLog as needed or have some
> > longer-lived object store the Log object.
> >
> > For StateHolder classes, things "just work"; the logger isn't saved in
> > saveState; it gets recreated in the new component and that works.
> >
> > For Serializable classes, the Log object handles its own serialization
> > correctly. No need for "transient", no need for any other special hacks.
> >
> > MyFaces does need to declare a dependency on commons-logging 1.0.4 (not
> > earlier versions) in order for things to work this simply. Release 1.0.4
> > was in June 2004.
> >
> > Do NOT use "static Log" anywhere.
> >
> > Is this all ok?
> >
> > Cheers,
> >
> > Simon
> >
> >
>
Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Manfred Geiler <ma...@gmail.com>.
+1 for dependency on commons-logging 1.0.4
BTW, Stan (Silvert), how do you solve these logging issues in JBoss?
AFAIK, JBoss has only one central log4j configuration. However, is
there a way to config logging per eapp or webapp? If yes, how does
JBoss address those issues with shared classes?
Thanks,
Manfred
On 2/21/06, Simon Kitching <sk...@apache.org> wrote:
> On Mon, 2006-02-20 at 21:47 -0800, Craig McClanahan wrote:
> > No, I was not aware of that change ... but does it actually work?
> > Declaring something Serializable is not by itself sufficient if there
> > are transient variables inside the implementation. (On a separate
> > thread on commons-dev, I recommended that there be unit tests to
> > validate this behavior: use a Log instance, serialize it, deserialize
> > it, and use it some more).
>
> I believe Serializable is correctly implemented for all standard
> adapters except possibly AvalonLog [will follow that up on commons-dev].
> The deserialized objects correctly locate a fresh "underlying" log
> object to forward calls to.
>
> Yes there need to be unit tests for this; I'll look for them and add
> some if not already present.
>
> >
> >
> > The 1.0.4 release occurred around June 2004 so perhaps there
> > are a few
> > containers out there capable of running MyFaces but which are
> > still on
> > JCL 1.0.3. In addition there might be a few custom Log
> > adapters around
> > which *might* not implement Serializable.
> >
> > However in view of the complicated/ugly workaround for
> > non-serializable
> > Log implementations, it seems ok to me for MyFaces to just
> > declare a
> > dependency on JCL 1.0.4. If people don't comply then there is
> > a very
> > obvious "NotSerializableException: logAdapterClassName"
> > message
> > generated. The fix is then to upgrade to 1.0.4 (which is
> > binary
> > compatible with earlier releases) or a later release.
> >
> > Comments/Opinions?
> >
> > +1 on declaring a dependency on Commons Logging 1.0.4 ... anything
> > prior to that is just asking for trouble. I use 1.0.4 exclusively for
> > eveything that has a C-L dependency ... works fine, lasts a long
> > time :-).
>
> So the conclusion seems to be that if/when using commons-logging in
> MyFaces, use:
> private Log foo = LogFactory.getLog("foo");
> everywhere except in short-lived and frequently-created objects. In
> those cases, either call LogFactory.getLog as needed or have some
> longer-lived object store the Log object.
>
> For StateHolder classes, things "just work"; the logger isn't saved in
> saveState; it gets recreated in the new component and that works.
>
> For Serializable classes, the Log object handles its own serialization
> correctly. No need for "transient", no need for any other special hacks.
>
> MyFaces does need to declare a dependency on commons-logging 1.0.4 (not
> earlier versions) in order for things to work this simply. Release 1.0.4
> was in June 2004.
>
> Do NOT use "static Log" anywhere.
>
> Is this all ok?
>
> Cheers,
>
> Simon
>
>
Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Simon Kitching <sk...@apache.org>.
On Mon, 2006-02-20 at 21:47 -0800, Craig McClanahan wrote:
> No, I was not aware of that change ... but does it actually work?
> Declaring something Serializable is not by itself sufficient if there
> are transient variables inside the implementation. (On a separate
> thread on commons-dev, I recommended that there be unit tests to
> validate this behavior: use a Log instance, serialize it, deserialize
> it, and use it some more).
I believe Serializable is correctly implemented for all standard
adapters except possibly AvalonLog [will follow that up on commons-dev].
The deserialized objects correctly locate a fresh "underlying" log
object to forward calls to.
Yes there need to be unit tests for this; I'll look for them and add
some if not already present.
>
>
> The 1.0.4 release occurred around June 2004 so perhaps there
> are a few
> containers out there capable of running MyFaces but which are
> still on
> JCL 1.0.3. In addition there might be a few custom Log
> adapters around
> which *might* not implement Serializable.
>
> However in view of the complicated/ugly workaround for
> non-serializable
> Log implementations, it seems ok to me for MyFaces to just
> declare a
> dependency on JCL 1.0.4. If people don't comply then there is
> a very
> obvious "NotSerializableException: logAdapterClassName"
> message
> generated. The fix is then to upgrade to 1.0.4 (which is
> binary
> compatible with earlier releases) or a later release.
>
> Comments/Opinions?
>
> +1 on declaring a dependency on Commons Logging 1.0.4 ... anything
> prior to that is just asking for trouble. I use 1.0.4 exclusively for
> eveything that has a C-L dependency ... works fine, lasts a long
> time :-).
So the conclusion seems to be that if/when using commons-logging in
MyFaces, use:
private Log foo = LogFactory.getLog("foo");
everywhere except in short-lived and frequently-created objects. In
those cases, either call LogFactory.getLog as needed or have some
longer-lived object store the Log object.
For StateHolder classes, things "just work"; the logger isn't saved in
saveState; it gets recreated in the new component and that works.
For Serializable classes, the Log object handles its own serialization
correctly. No need for "transient", no need for any other special hacks.
MyFaces does need to declare a dependency on commons-logging 1.0.4 (not
earlier versions) in order for things to work this simply. Release 1.0.4
was in June 2004.
Do NOT use "static Log" anywhere.
Is this all ok?
Cheers,
Simon
Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Craig McClanahan <cr...@apache.org>.
On 2/20/06, Simon Kitching <sk...@apache.org> wrote:
>
> Hi Craig,
>
> On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
> > There *is* a JSF-specific consideration to think about, if you have
> > classes that implement StateHolder (like a UIComponent
> > implementation). Log instances will generally *not* be serializable,
> > so you will need to deal specially with them in saveState() and
> > restoreState() methods. The simplest thing is to just not save them,
> > and do a "new" operation again in restoreState().
> >
> > Along the same lines, if your class implements Serializable, you will
> > need to mark the instance variable transient. I've started using the
> > following pattern in my Serializable classes, which would work inside
> > a StateHolder as well:
>
> > private transient Log log = null;
> >
> > private Log log() {
> > if (log == null) {
> > log = LogFactory.getLog(...);
> > }
> > return log;
> > }
> >
> > and a typical call looks like:
> >
> > if (log().isDebugEnabled()) {
> > log().debug("...");
> > }
>
>
> I've checked, and all the standard commons-logging Log adapter
> implementations *are* Serializable (Log4J, JDK14, LogKit, Simple, NoOp).
> So AIUI there is no need to declare Log members transient; they should
> get serialized and deserialized along with the owning class fine. No
> log() method would then be required or any other special handling (other
> than ensuring the member is NOT static!).
>
> Serializable support for Log implementations appears to have been
> implemented in the JCL 1.0.4 release, ie in JCL 1.0.3 and earlier the
> Log implementations were not Serializable. Craig: maybe you weren't
> aware of this change?
No, I was not aware of that change ... but does it actually work? Declaring
something Serializable is not by itself sufficient if there are transient
variables inside the implementation. (On a separate thread on commons-dev,
I recommended that there be unit tests to validate this behavior: use a Log
instance, serialize it, deserialize it, and use it some more).
The 1.0.4 release occurred around June 2004 so perhaps there are a few
> containers out there capable of running MyFaces but which are still on
> JCL 1.0.3. In addition there might be a few custom Log adapters around
> which *might* not implement Serializable.
>
> However in view of the complicated/ugly workaround for non-serializable
> Log implementations, it seems ok to me for MyFaces to just declare a
> dependency on JCL 1.0.4. If people don't comply then there is a very
> obvious "NotSerializableException: logAdapterClassName" message
> generated. The fix is then to upgrade to 1.0.4 (which is binary
> compatible with earlier releases) or a later release.
>
> Comments/Opinions?
+1 on declaring a dependency on Commons Logging 1.0.4 ... anything prior to
that is just asking for trouble. I use 1.0.4 exclusively for eveything that
has a C-L dependency ... works fine, lasts a long time :-).
Cheers,
>
> Simon
>
>
Craig
Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Simon Kitching <sk...@apache.org>.
Hi Craig,
On Sun, 2006-02-19 at 17:56 -0800, Craig McClanahan wrote:
> There *is* a JSF-specific consideration to think about, if you have
> classes that implement StateHolder (like a UIComponent
> implementation). Log instances will generally *not* be serializable,
> so you will need to deal specially with them in saveState() and
> restoreState() methods. The simplest thing is to just not save them,
> and do a "new" operation again in restoreState().
>
> Along the same lines, if your class implements Serializable, you will
> need to mark the instance variable transient. I've started using the
> following pattern in my Serializable classes, which would work inside
> a StateHolder as well:
> private transient Log log = null;
>
> private Log log() {
> if (log == null) {
> log = LogFactory.getLog(...);
> }
> return log;
> }
>
> and a typical call looks like:
>
> if (log().isDebugEnabled()) {
> log().debug("...");
> }
I've checked, and all the standard commons-logging Log adapter
implementations *are* Serializable (Log4J, JDK14, LogKit, Simple, NoOp).
So AIUI there is no need to declare Log members transient; they should
get serialized and deserialized along with the owning class fine. No
log() method would then be required or any other special handling (other
than ensuring the member is NOT static!).
Serializable support for Log implementations appears to have been
implemented in the JCL 1.0.4 release, ie in JCL 1.0.3 and earlier the
Log implementations were not Serializable. Craig: maybe you weren't
aware of this change?
The 1.0.4 release occurred around June 2004 so perhaps there are a few
containers out there capable of running MyFaces but which are still on
JCL 1.0.3. In addition there might be a few custom Log adapters around
which *might* not implement Serializable.
However in view of the complicated/ugly workaround for non-serializable
Log implementations, it seems ok to me for MyFaces to just declare a
dependency on JCL 1.0.4. If people don't comply then there is a very
obvious "NotSerializableException: logAdapterClassName" message
generated. The fix is then to upgrade to 1.0.4 (which is binary
compatible with earlier releases) or a later release.
Comments/Opinions?
Cheers,
Simon
Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Craig McClanahan <cr...@apache.org>.
On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
>
> On Sun, 2006-02-19 at 20:27 +0100, Manfred Geiler wrote:
> > On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > > Just a warning to all developers: when using commons-logging in a
> > > library, STATIC fields must **NOT** be used to store Log objects.
> > >
> > > The problem is that the class may be called with various
> thread-context
> > > classloaders (TCCL) set. However the class is only ever loaded once.
> >
> > Never heard of that. Simon are you really sure?
> > I think the opposite is true. Containers tend to have Classloaders
> > that all have there own set of loaded classes. So you get problems
> > when your applications Classloader holds some Classes that the
> > container itself has already loaded. Then you get such things like
> > "incomptible class" Exceptions and NoClassDefFound when a class in
> > your webapp is linked to another class in your app but the Loader
> > finds the containers class first.
>
> Yes, I'm really sure.
>
> Here's the relevant paragraph from the user-guide for the upcoming 1.1
> release [written by me]:
> http://jakarta.apache.org/commons/logging/guide.html#Developing%20With%
> 20JCL
>
> This is not about class compatibility issues; using instance members
> rather than class members doesn't change the classpath/classloader/etc.
>
> It's about the fact that when the field is static, there is only *one*
> Log instance for that class but multiple webapps are using it. Once
> constructed, the Log object simply points directly to a *configured*
> object within the underlying concrete logging library. That underlying
> object therefore either:
> (a) has no webapp-specific state (ie is initialised via config info
> associated with the container not a TCCL)
> (b) has the state of the first webapp that initialised it (ie is
> initialised via config info located via the TCCL active when the Log
> object was created).
>
> Option (a) occurs if the first call to the class is from the container
> itself, or if the concrete logging library used is in a "shared"
> classpath, or if TCCL-based configuration is not enabled in
> commons-logging. In this case, using a static Log is actually ok.
>
> Option (b) occurs when TCCL-based configuration is enabled, and a webapp
> is the first to access that class.
>
> The latter case is the usual one, resulting in logging output generated
> *as a result of webapp B calling MyFaces* ending up in the logfiles for
> *webapp A*. Not good.
>
> Any logging library that attempts to direct output from shared classes
> into the logfile of the webapp *it is running on behalf of* will have
> this issue. Log4j doesn't have this problem by default, as it doesn't
> attempt to put output from shared classes into webapp-specific logfiles.
> However it *does* have an optional mechanism to do this: the
> RepositorySelector.
> http://www.qos.ch/logging/sc.jsp
> If the "Contextual Repository Selector" approach is used as documented
> in the above page, then exactly the same benefits and problems occur in
> log4j as in commons-logging; logging by shared classes goes into the
> relevant webapp logfile [good] but static Log objects in shared classes
> can output to the wrong logfile [bad].
>
> Avoiding static Log instances in potentially shared classes is the only
> reliable solution.
>
> And as I mentioned in the earlier email, static fields in shared classes
> should be regarded with great suspicion in *all* cases, not just
> logging!
Simon,
Could you do me a favor and publicize this in the Struts community as well?
The framework code there is littered with static log instances to. You
might also want to add some notes related to using Log instances in
Serializable classes (see below).
MyFaces folks,
There *is* a JSF-specific consideration to think about, if you have classes
that implement StateHolder (like a UIComponent implementation). Log
instances will generally *not* be serializable, so you will need to deal
specially with them in saveState() and restoreState() methods. The simplest
thing is to just not save them, and do a "new" operation again in
restoreState().
Along the same lines, if your class implements Serializable, you will need
to mark the instance variable transient. I've started using the following
pattern in my Serializable classes, which would work inside a StateHolder as
well:
private transient Log log = null;
private Log log() {
if (log == null) {
log = LogFactory.getLog(...);
}
return log;
}
and a typical call looks like:
if (log().isDebugEnabled()) {
log().debug("...");
}
Regards,
>
> Simon
>
>
Craig
Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Martin Marinschek <ma...@gmail.com>.
wo-ow.
it's great to learn a new thing every day ;)
regards,
Martin
On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> On Sun, 2006-02-19 at 20:27 +0100, Manfred Geiler wrote:
> > On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > > Just a warning to all developers: when using commons-logging in a
> > > library, STATIC fields must **NOT** be used to store Log objects.
> > >
> > > The problem is that the class may be called with various thread-context
> > > classloaders (TCCL) set. However the class is only ever loaded once.
> >
> > Never heard of that. Simon are you really sure?
> > I think the opposite is true. Containers tend to have Classloaders
> > that all have there own set of loaded classes. So you get problems
> > when your applications Classloader holds some Classes that the
> > container itself has already loaded. Then you get such things like
> > "incomptible class" Exceptions and NoClassDefFound when a class in
> > your webapp is linked to another class in your app but the Loader
> > finds the containers class first.
>
> Yes, I'm really sure.
>
> Here's the relevant paragraph from the user-guide for the upcoming 1.1
> release [written by me]:
> http://jakarta.apache.org/commons/logging/guide.html#Developing%20With%
> 20JCL
>
> This is not about class compatibility issues; using instance members
> rather than class members doesn't change the classpath/classloader/etc.
>
> It's about the fact that when the field is static, there is only *one*
> Log instance for that class but multiple webapps are using it. Once
> constructed, the Log object simply points directly to a *configured*
> object within the underlying concrete logging library. That underlying
> object therefore either:
> (a) has no webapp-specific state (ie is initialised via config info
> associated with the container not a TCCL)
> (b) has the state of the first webapp that initialised it (ie is
> initialised via config info located via the TCCL active when the Log
> object was created).
>
> Option (a) occurs if the first call to the class is from the container
> itself, or if the concrete logging library used is in a "shared"
> classpath, or if TCCL-based configuration is not enabled in
> commons-logging. In this case, using a static Log is actually ok.
>
> Option (b) occurs when TCCL-based configuration is enabled, and a webapp
> is the first to access that class.
>
> The latter case is the usual one, resulting in logging output generated
> *as a result of webapp B calling MyFaces* ending up in the logfiles for
> *webapp A*. Not good.
>
> Any logging library that attempts to direct output from shared classes
> into the logfile of the webapp *it is running on behalf of* will have
> this issue. Log4j doesn't have this problem by default, as it doesn't
> attempt to put output from shared classes into webapp-specific logfiles.
> However it *does* have an optional mechanism to do this: the
> RepositorySelector.
> http://www.qos.ch/logging/sc.jsp
> If the "Contextual Repository Selector" approach is used as documented
> in the above page, then exactly the same benefits and problems occur in
> log4j as in commons-logging; logging by shared classes goes into the
> relevant webapp logfile [good] but static Log objects in shared classes
> can output to the wrong logfile [bad].
>
> Avoiding static Log instances in potentially shared classes is the only
> reliable solution.
>
> And as I mentioned in the earlier email, static fields in shared classes
> should be regarded with great suspicion in *all* cases, not just
> logging!
>
>
> Regards,
>
> Simon
>
>
--
http://www.irian.at
Your JSF powerhouse -
JSF Consulting, Development and
Courses in English and German
Professional Support for Apache MyFaces
Re: svn commit: r378805 -
/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Simon Kitching <sk...@apache.org>.
On Sun, 2006-02-19 at 20:27 +0100, Manfred Geiler wrote:
> On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> > Just a warning to all developers: when using commons-logging in a
> > library, STATIC fields must **NOT** be used to store Log objects.
> >
> > The problem is that the class may be called with various thread-context
> > classloaders (TCCL) set. However the class is only ever loaded once.
>
> Never heard of that. Simon are you really sure?
> I think the opposite is true. Containers tend to have Classloaders
> that all have there own set of loaded classes. So you get problems
> when your applications Classloader holds some Classes that the
> container itself has already loaded. Then you get such things like
> "incomptible class" Exceptions and NoClassDefFound when a class in
> your webapp is linked to another class in your app but the Loader
> finds the containers class first.
Yes, I'm really sure.
Here's the relevant paragraph from the user-guide for the upcoming 1.1
release [written by me]:
http://jakarta.apache.org/commons/logging/guide.html#Developing%20With%
20JCL
This is not about class compatibility issues; using instance members
rather than class members doesn't change the classpath/classloader/etc.
It's about the fact that when the field is static, there is only *one*
Log instance for that class but multiple webapps are using it. Once
constructed, the Log object simply points directly to a *configured*
object within the underlying concrete logging library. That underlying
object therefore either:
(a) has no webapp-specific state (ie is initialised via config info
associated with the container not a TCCL)
(b) has the state of the first webapp that initialised it (ie is
initialised via config info located via the TCCL active when the Log
object was created).
Option (a) occurs if the first call to the class is from the container
itself, or if the concrete logging library used is in a "shared"
classpath, or if TCCL-based configuration is not enabled in
commons-logging. In this case, using a static Log is actually ok.
Option (b) occurs when TCCL-based configuration is enabled, and a webapp
is the first to access that class.
The latter case is the usual one, resulting in logging output generated
*as a result of webapp B calling MyFaces* ending up in the logfiles for
*webapp A*. Not good.
Any logging library that attempts to direct output from shared classes
into the logfile of the webapp *it is running on behalf of* will have
this issue. Log4j doesn't have this problem by default, as it doesn't
attempt to put output from shared classes into webapp-specific logfiles.
However it *does* have an optional mechanism to do this: the
RepositorySelector.
http://www.qos.ch/logging/sc.jsp
If the "Contextual Repository Selector" approach is used as documented
in the above page, then exactly the same benefits and problems occur in
log4j as in commons-logging; logging by shared classes goes into the
relevant webapp logfile [good] but static Log objects in shared classes
can output to the wrong logfile [bad].
Avoiding static Log instances in potentially shared classes is the only
reliable solution.
And as I mentioned in the earlier email, static fields in shared classes
should be regarded with great suspicion in *all* cases, not just
logging!
Regards,
Simon
Re: svn commit: r378805 - /myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
Posted by Manfred Geiler <ma...@gmail.com>.
On 2/19/06, Simon Kitching <sk...@apache.org> wrote:
> On Sun, 2006-02-19 at 00:46 +0000, mmarinschek@apache.org wrote:
> > Author: mmarinschek
> > Date: Sat Feb 18 16:46:18 2006
> > New Revision: 378805
> >
> > URL: http://svn.apache.org/viewcvs?rev=378805&view=rev
> > Log:
> > minor changes in application-factory, fixed "readOnly" referral in HtmlRendererUtils
> >
> > Modified:
> > myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
> >
> > Modified: myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java
> > URL: http://svn.apache.org/viewcvs/myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java?rev=378805&r1=378804&r2=378805&view=diff
> > ==============================================================================
> > --- myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java (original)
> > +++ myfaces/core/trunk/impl/src/main/java/org/apache/myfaces/application/ApplicationFactoryImpl.java Sat Feb 18 16:46:18 2006
> > @@ -29,12 +29,14 @@
> > public class ApplicationFactoryImpl
> > extends ApplicationFactory
> > {
> > - private static final Log log = LogFactory.getLog(ApplicationFactory.class);
> > + private static final Log log = LogFactory.getLog(ApplicationFactoryImpl.class);
>
>
> Just a warning to all developers: when using commons-logging in a
> library, STATIC fields must **NOT** be used to store Log objects.
>
> The problem is that the class may be called with various thread-context
> classloaders (TCCL) set. However the class is only ever loaded once.
Never heard of that. Simon are you really sure?
I think the opposite is true. Containers tend to have Classloaders
that all have there own set of loaded classes. So you get problems
when your applications Classloader holds some Classes that the
container itself has already loaded. Then you get such things like
"incomptible class" Exceptions and NoClassDefFound when a class in
your webapp is linked to another class in your app but the Loader
finds the containers class first.
Manfred
> If the Log object is static, then it is initialised using the TCCL of
> the very first webapp to use it (ie which forces the class to be loaded)
> - including picking up config info from that TCCL. All other webapps
> will then use the same Log object - which is not healthy.
>
> Yes this is a nuisance, but it's absolutely necessary to avoid storing
> Log instances in static fields for libraries like MyFaces.
>
> Regards,
>
> Simon
>
>