Skip to content

Handle missing stack traces in ExtendedThreadInformation #3655

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

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,17 @@
*/
package org.apache.logging.log4j.core.message;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.lang.management.ThreadInfo;
import org.apache.logging.log4j.message.ThreadDumpMessage;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.EnumSource;

/**
* Tests that ThreadDumpMessage uses ExtendedThreadInformation when available.
Expand All @@ -33,4 +40,42 @@ void testMessage() {
// System.out.print(message);
assertTrue(message.contains(" Id="), "No header");
}

@ParameterizedTest
@EnumSource(Thread.State.class)
void testMessageWithNullStackTrace(final Thread.State state) {
obtainMessageWithMissingStackTrace(state, null);
}

@ParameterizedTest
@EnumSource(Thread.State.class)
void testMessageWithEmptyStackTrace(final Thread.State state) {
obtainMessageWithMissingStackTrace(state, new StackTraceElement[0]);
}

private void obtainMessageWithMissingStackTrace(final Thread.State state, final StackTraceElement[] stackTrace) {
// setup
final String threadName = "the thread name";
final long threadId = 23523L;

final ThreadInfo threadInfo = mock(ThreadInfo.class);
when(threadInfo.getStackTrace()).thenReturn(stackTrace);
when(threadInfo.getThreadName()).thenReturn(threadName);
when(threadInfo.getThreadId()).thenReturn(threadId);
when(threadInfo.isSuspended()).thenReturn(true);
when(threadInfo.isInNative()).thenReturn(true);
when(threadInfo.getThreadState()).thenReturn(state);

// given
final ExtendedThreadInformation sut = new ExtendedThreadInformation(threadInfo);

// when
final StringBuilder result = new StringBuilder();
sut.printThreadInfo(result);

// then
assertThat(result.toString(), containsString(threadName));
assertThat(result.toString(), containsString(state.name()));
assertThat(result.toString(), containsString(String.valueOf(threadId)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ private void formatState(final StringBuilder sb, final ThreadInfo info) {
break;
}
case WAITING: {
final StackTraceElement element = info.getStackTrace()[0];
final StackTraceElement element = getStackTraceElement(info);
final String className = element.getClassName();
final String method = element.getMethodName();
if (className.equals("java.lang.Object") && method.equals("wait")) {
Expand All @@ -144,7 +144,7 @@ private void formatState(final StringBuilder sb, final ThreadInfo info) {
break;
}
case TIMED_WAITING: {
final StackTraceElement element = info.getStackTrace()[0];
final StackTraceElement element = getStackTraceElement(info);
final String className = element.getClassName();
final String method = element.getMethodName();
if (className.equals("java.lang.Object") && method.equals("wait")) {
Expand Down Expand Up @@ -174,4 +174,14 @@ private void formatState(final StringBuilder sb, final ThreadInfo info) {
break;
}
}

private static StackTraceElement getStackTraceElement(final ThreadInfo info) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you implement the following changes, please?

  1. Rename this method to getFirstStackTraceElement
  2. Return null on empty stack trace (instead of returning a dummy StackTraceElement with a magic value in it) and mark the method with @Nullable from JSpecify
  3. Update above case WAITING and case TIMED_WAITING blocks accordingly, e.g.,
final StackTraceElement element = getFirstStackTraceElement(info);
final String className = element != null ? element.getClassName() : null;
final String method = element != null ? element.getMethodName() : null;
if ("java.lang.Object".equals(className) && "wait".equals(method)) {
    ...

final StackTraceElement[] stackTrace = info.getStackTrace();
if (stackTrace == null || stackTrace.length < 1) {
final String textualInfo = "Unknown";
return new StackTraceElement(textualInfo, textualInfo, textualInfo, -1);
}

return stackTrace[0];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?xml version="1.0" encoding="UTF-8"?>
<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xmlns="https://logging.apache.org/xml/ns"
xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
type="fixed">
<issue id="3655" link="https://github.com/apache/logging-log4j2/pull/3655"/>
<description format="asciidoc">
Fix `ArrayIndexOutOfBoundsException` on invocation of `Message.getFormattedMessage()` when any thread has no stack trace, which occurs on some JVM implementations.
</description>
</entry>