This is an automated email from the ASF dual-hosted git repository. pkarwasz pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/logging-log4j2.git
commit 3003538519e8bfb06e99468f837d9495b9eb4f58 Author: Aliaksei Dubrouski <[email protected]> AuthorDate: Wed Jan 18 16:21:39 2023 -0800 Addressing PR feedback. Adding changelog item, fixing slow path in StackLocator and unit test for correct and wrong order of elements in the stack trace --- .../apache/logging/log4j/util/StackLocator.java | 1 - .../log4j/core/impl/ThrowableProxyHelperTest.java | 85 ++++++++++++++++++++++ src/changelog/.2.x.x/1214_fix_stacktrace_order.xml | 22 ++++++ 3 files changed, 107 insertions(+), 1 deletion(-) diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/StackLocator.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/StackLocator.java index a9f562cfda..5f68fc58f5 100644 --- a/log4j-api/src/main/java/org/apache/logging/log4j/util/StackLocator.java +++ b/log4j-api/src/main/java/org/apache/logging/log4j/util/StackLocator.java @@ -108,7 +108,6 @@ public final class StackLocator { s.forEach(f -> stack.add(f.getDeclaringClass())); return stack; }); - } public StackTraceElement calcLocation(final String fqcnOfLogger) { diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/ThrowableProxyHelperTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/ThrowableProxyHelperTest.java new file mode 100644 index 0000000000..5575fb5473 --- /dev/null +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/ThrowableProxyHelperTest.java @@ -0,0 +1,85 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache license, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the license for the specific language governing permissions and + * limitations under the license. + */ +package org.apache.logging.log4j.core.impl; + +import java.util.ArrayDeque; +import java.util.Deque; +import java.util.HashMap; +import java.util.Map; +import org.junit.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.assertFalse; + +/** + * Tests ThrowableProxyHelper. + */ +public class ThrowableProxyHelperTest { + + /** + * We populate dummy stack trace and array of stack trace elements in the right order + * It supposed to always trigger fast path so cache won't be populated + * This simulates the case when current thread's and throwable stack traces have the same elements + */ + @Test + public void testSuccessfulCacheHit() { + final Map<String, ThrowableProxyHelper.CacheEntry> map = new HashMap<>(); + final Deque<Class<?>> stack = new ArrayDeque<>(3); + StackTraceElement[] stackTraceElements = new StackTraceElement[3]; + stackTraceElements[0] = new StackTraceElement(Integer.class.getName(), "toString", + "Integer.java", 1); + stack.addLast(Integer.class); + stackTraceElements[1] = new StackTraceElement(Float.class.getName(), "toString", + "Float.java", 1); + stack.addLast(Float.class); + stackTraceElements[2] = new StackTraceElement(Double.class.getName(), "toString", + "Double.java", 1); + stack.addLast(Double.class); + final Throwable throwable = new IllegalStateException("This is a test"); + final ThrowableProxy proxy = new ThrowableProxy(throwable); + ThrowableProxyHelper.toExtendedStackTrace(proxy, stack, map, null, stackTraceElements); + assertTrue(map.isEmpty()); + } + + /** + * We populate dummy stack trace and array of stack trace elements in the wrong order + * It will trigger fast path only once so cache will have two items + */ + @Test + public void testFailedCacheHit() { + final Map<String, ThrowableProxyHelper.CacheEntry> map = new HashMap<>(); + final Deque<Class<?>> stack = new ArrayDeque<>(3); + StackTraceElement[] stackTraceElements = new StackTraceElement[3]; + stackTraceElements[0] = new StackTraceElement(Integer.class.getName(), "toString", + "Integer.java", 1); + stack.addFirst(Integer.class); + stackTraceElements[1] = new StackTraceElement(Float.class.getName(), "toString", + "Float.java", 1); + stack.addFirst(Float.class); + stackTraceElements[2] = new StackTraceElement(Double.class.getName(), "toString", + "Double.java", 1); + stack.addFirst(Double.class); + final Throwable throwable = new IllegalStateException("This is a test"); + final ThrowableProxy proxy = new ThrowableProxy(throwable); + ThrowableProxyHelper.toExtendedStackTrace(proxy, stack, map, null, stackTraceElements); + assertFalse(map.isEmpty()); + //Integer will match, so fast path won't cache it, only Float and Double will appear in cache after class loading + assertTrue(map.containsKey(Double.class.getName())); + assertTrue(map.containsKey(Float.class.getName())); + } + +} diff --git a/src/changelog/.2.x.x/1214_fix_stacktrace_order.xml b/src/changelog/.2.x.x/1214_fix_stacktrace_order.xml new file mode 100644 index 0000000000..baee854ae1 --- /dev/null +++ b/src/changelog/.2.x.x/1214_fix_stacktrace_order.xml @@ -0,0 +1,22 @@ +<?xml version="1.0" encoding="UTF-8"?> +<!-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--> +<entry type="changed"> + <issue id="1214" link="https://github.com/apache/logging-log4j2/pull/1214"/> + <author id="vy"/> + <description format="asciidoc">Fixing the issue with stacktrace elements order which causes major regression due to cache misses</description> +</entry>
