Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ISPN-8647 Initialize all loggers statically #5660

Merged
merged 2 commits into from Jan 10, 2018

Conversation

tristantarrant
Copy link
Member

This prevents a potential deadlock in the Logger factory which synchronizes on the ClassLoader.

https://issues.jboss.org/browse/ISPN-8647

@tristantarrant tristantarrant added this to the 9.2.0.CR1 milestone Jan 8, 2018
@rvansa
Copy link
Member

rvansa commented Jan 8, 2018

@tristantarrant While I agree with making the loggers static (despite losing info about inheriting class), I don't think that the output is equivalent to the old behaviour. Simple test tells me:

import java.lang.invoke.MethodHandles;

public class LookupTest {
   static class A {
      static final Class<?> clazz = MethodHandles.lookup().lookupClass();

      public void foo() {
         System.out.println(clazz.getName());
      }
   }

   static class B extends A {
   }

   public static void main(String[] args) {
      new B().foo();
   }
}

and the output is LookupTest$A. If this is intended you don't have to use MethodHandles, you can simply state the class directly.

Copy link
Member

@wburns wburns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few tweaks and it should be a bit cleaner.

@@ -39,7 +40,7 @@
* @since 9.0
*/
public class OffHeapDataContainer implements DataContainer<WrappedBytes, WrappedBytes> {
protected final Log log = LogFactory.getLog(getClass());
protected static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately before this was used to show trace messages from the appropriate implementation. I have had to fix this before and the way to do this is to instead add an abstract getLog method to the abstract class that is implemented by the extended classes to properly say where the messages are from. This way the extending class can still define the logger as static but the abstract class uses the appropriate logger.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -26,7 +27,7 @@
* @since 7.0
*/
abstract class BaseQueueingSegmentListener<K, V, E extends Event<K, V>> implements QueueingSegmentListener<K, V, E> {
protected final Log log = LogFactory.getLog(getClass());
protected static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can use the getLog approach as well.

@@ -66,7 +67,7 @@
* @param <S> The stream interface
*/
public abstract class AbstractCacheStream<T, S extends BaseStream<T, S>, S2 extends S> implements BaseStream<T, S> {
protected final Log log = LogFactory.getLog(getClass());
protected static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually this line can be completely removed (I missed removing it on a refactor). This is already using the getLog approach I mentioned on the off heap one.

@@ -19,7 +20,7 @@
* stream is populated
*/
public abstract class AbstractLocalCacheStream<T, S extends BaseStream<T, S>, S2 extends S> implements BaseStream<T, S> {
protected final Log log = LogFactory.getLog(getClass());
protected static final Log log = LogFactory.getLog(MethodHandles.lookup().lookupClass());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line can be removed - unused.

@tristantarrant
Copy link
Member Author

I have used the getLog() approach where inheritance matters.

@wburns wburns merged commit 120c755 into infinispan:master Jan 10, 2018
@wburns
Copy link
Member

wburns commented Jan 10, 2018

Integrated into master, thanks @tristantarrant !

@tristantarrant tristantarrant deleted the ISPN-8647/deadlog branch April 10, 2018 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants