Skip to content

Commit

Permalink
Fix StatusLogger time-zone issues and stack overflow (#2322)
Browse files Browse the repository at this point in the history
* Add time-zone support to `StatusLogger`. Without a time-zone,
  it is not possible to format certain date & time fields that are
  time-zone-specific, e.g., year-of-era.

* Make sure `StatusLogger#logMessage()` failures don't cause
  stack overflow
  • Loading branch information
vy committed Feb 27, 2024
1 parent 6d407b6 commit af045db
Show file tree
Hide file tree
Showing 10 changed files with 640 additions and 39 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* 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.status;

import static org.apache.logging.log4j.status.StatusLogger.DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY;
import static org.assertj.core.api.Assertions.assertThat;

import java.util.Properties;
import org.junit.jupiter.api.Test;
import uk.org.webcompere.systemstubs.SystemStubs;

class StatusLoggerBufferCapacityTest {

@Test
void valid_buffer_capacity_should_be_effective() {

// Create a `StatusLogger` configuration
final Properties statusLoggerConfigProperties = new Properties();
final int bufferCapacity = 10;
assertThat(bufferCapacity).isNotEqualTo(DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY);
statusLoggerConfigProperties.put(StatusLogger.MAX_STATUS_ENTRIES, "" + bufferCapacity);
final StatusLogger.Config statusLoggerConfig = new StatusLogger.Config(statusLoggerConfigProperties);

// Verify the buffer capacity
assertThat(statusLoggerConfig.bufferCapacity).isEqualTo(bufferCapacity);
}

@Test
void invalid_buffer_capacity_should_cause_fallback_to_defaults() throws Exception {

// Create a `StatusLogger` configuration using an invalid buffer capacity
final Properties statusLoggerConfigProperties = new Properties();
final int invalidBufferCapacity = -10;
statusLoggerConfigProperties.put(StatusLogger.MAX_STATUS_ENTRIES, "" + invalidBufferCapacity);
final StatusLogger.Config[] statusLoggerConfigRef = {null};
final String stderr = SystemStubs.tapSystemErr(
() -> statusLoggerConfigRef[0] = new StatusLogger.Config(statusLoggerConfigProperties));
final StatusLogger.Config statusLoggerConfig = statusLoggerConfigRef[0];

// Verify the stderr dump
assertThat(stderr).contains("Failed reading the buffer capacity");

// Verify the buffer capacity
assertThat(statusLoggerConfig.bufferCapacity).isEqualTo(DEFAULT_FALLBACK_LISTENER_BUFFER_CAPACITY);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,105 @@
/*
* 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.status;

import static org.assertj.core.api.Assertions.assertThat;

import edu.umd.cs.findbugs.annotations.Nullable;
import java.time.Instant;
import java.time.ZoneId;
import java.time.format.DateTimeFormatter;
import java.util.Properties;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.CsvSource;
import uk.org.webcompere.systemstubs.SystemStubs;

class StatusLoggerDateTest {

@ParameterizedTest
@CsvSource({"yyyy-MM-dd", "HH:mm:ss", "HH:mm:ss.SSS"})
void common_date_patterns_should_work(final String instantPattern) {

// Create a `StatusLogger` configuration
final Properties statusLoggerConfigProperties = new Properties();
statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT, instantPattern);
final ZoneId zoneId = ZoneId.of("UTC");
statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT_ZONE, zoneId.toString());
final StatusLogger.Config statusLoggerConfig = new StatusLogger.Config(statusLoggerConfigProperties);

// Verify the formatter
final DateTimeFormatter formatter =
DateTimeFormatter.ofPattern(instantPattern).withZone(zoneId);
verifyFormatter(statusLoggerConfig.instantFormatter, formatter);
}

@Test
void invalid_date_format_should_cause_fallback_to_defaults() throws Exception {
final String invalidFormat = "l";
verifyInvalidDateFormatAndZone(invalidFormat, "UTC", "failed reading the instant format", null);
}

@Test
void invalid_date_format_zone_should_cause_fallback_to_defaults() throws Exception {
final String invalidZone = "XXX";
final String format = "yyyy";
verifyInvalidDateFormatAndZone(
format,
invalidZone,
"Failed reading the instant formatting zone ID",
DateTimeFormatter.ofPattern(format).withZone(ZoneId.systemDefault()));
}

private static void verifyInvalidDateFormatAndZone(
final String format,
final String zone,
final String stderrMessage,
@Nullable final DateTimeFormatter formatter)
throws Exception {

// Create a `StatusLogger` configuration using invalid input
final Properties statusLoggerConfigProperties = new Properties();
statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT, format);
statusLoggerConfigProperties.put(StatusLogger.STATUS_DATE_FORMAT_ZONE, zone);
final StatusLogger.Config[] statusLoggerConfigRef = {null};
final String stderr = SystemStubs.tapSystemErr(
() -> statusLoggerConfigRef[0] = new StatusLogger.Config(statusLoggerConfigProperties));
final StatusLogger.Config statusLoggerConfig = statusLoggerConfigRef[0];

// Verify the stderr dump
assertThat(stderr).contains(stderrMessage);

// Verify the formatter
verifyFormatter(statusLoggerConfig.instantFormatter, formatter);
}

/**
* {@link DateTimeFormatter} doesn't have an {@link Object#equals(Object)} implementation, hence <a href="https://stackoverflow.com/a/63887712/1278899">this manual <em>behavioral</em> comparison</a>.
*
* @param actual the actual formatter
* @param expected the expected formatter
*/
private static void verifyFormatter(@Nullable DateTimeFormatter actual, @Nullable DateTimeFormatter expected) {
if (expected == null) {
assertThat(actual).isNull();
} else {
assertThat(actual).isNotNull();
final Instant instant = Instant.now();
assertThat(actual.format(instant)).isEqualTo(expected.format(instant));
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* 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.status;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import org.apache.logging.log4j.Level;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.ExtendWith;
import org.junit.jupiter.api.parallel.ResourceLock;
import uk.org.webcompere.systemstubs.SystemStubs;
import uk.org.webcompere.systemstubs.jupiter.SystemStubsExtension;

@ExtendWith(SystemStubsExtension.class)
@ResourceLock("log4j2.StatusLogger")
class StatusLoggerFailingListenerTest {

public static final StatusLogger STATUS_LOGGER = StatusLogger.getLogger();

private StatusListener listener;

@BeforeEach
void createAndRegisterListener() {
listener = mock(StatusListener.class);
STATUS_LOGGER.registerListener(listener);
}

@AfterEach
void unregisterListener() {
STATUS_LOGGER.removeListener(listener);
}

@Test
void logging_with_failing_listener_should_not_cause_stack_overflow() throws Exception {

// Set up a failing listener on `log(StatusData)`
when(listener.getStatusLevel()).thenReturn(Level.ALL);
final Exception listenerFailure = new RuntimeException("test failure " + Math.random());
doThrow(listenerFailure).when(listener).log(any());

// Log something and verify exception dump
final String stderr = SystemStubs.tapSystemErr(() -> STATUS_LOGGER.error("foo"));
final String listenerFailureClassName = listenerFailure.getClass().getCanonicalName();
assertThat(stderr).contains(listenerFailureClassName + ": " + listenerFailure.getMessage());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,21 +16,24 @@
*/
package org.apache.logging.log4j.status;

import static org.apache.logging.log4j.status.StatusLogger.DEFAULT_FALLBACK_LISTENER_LEVEL;
import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import java.util.Properties;
import org.apache.logging.log4j.Level;
import org.junit.jupiter.api.Test;
import uk.org.webcompere.systemstubs.SystemStubs;

class StatusLoggerLevelTest {

@Test
void effective_level_should_be_the_least_specific_one() {

// Verify the initial level
final StatusLogger logger = StatusLogger.getLogger();
final Level fallbackListenerLevel = Level.ERROR;
final StatusLogger logger = new StatusLogger();
final Level fallbackListenerLevel = DEFAULT_FALLBACK_LISTENER_LEVEL;
assertThat(logger.getLevel()).isEqualTo(fallbackListenerLevel);

// Register a less specific listener
Expand Down Expand Up @@ -82,4 +85,23 @@ void effective_level_should_be_the_least_specific_one() {
logger.removeListener(listener1);
assertThat(logger.getLevel()).isEqualTo(fallbackListenerLevel); // Verify that the level is changed
}

@Test
void invalid_level_should_cause_fallback_to_defaults() throws Exception {

// Create a `StatusLogger` configuration using an invalid level
final Properties statusLoggerConfigProperties = new Properties();
final String invalidLevelName = "FOO";
statusLoggerConfigProperties.put(StatusLogger.DEFAULT_STATUS_LISTENER_LEVEL, invalidLevelName);
final StatusLogger.Config[] statusLoggerConfigRef = {null};
final String stderr = SystemStubs.tapSystemErr(
() -> statusLoggerConfigRef[0] = new StatusLogger.Config(statusLoggerConfigProperties));
final StatusLogger.Config statusLoggerConfig = statusLoggerConfigRef[0];

// Verify the stderr dump
assertThat(stderr).contains("Failed reading the level");

// Verify the level
assertThat(statusLoggerConfig.fallbackListenerLevel).isEqualTo(DEFAULT_FALLBACK_LISTENER_LEVEL);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
/*
* 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.status;

import static org.apache.logging.log4j.status.StatusLogger.PropertiesUtilsDouble.readAllAvailableProperties;
import static org.apache.logging.log4j.status.StatusLogger.PropertiesUtilsDouble.readProperty;
import static org.assertj.core.api.Assertions.assertThat;
import static uk.org.webcompere.systemstubs.SystemStubs.restoreSystemProperties;
import static uk.org.webcompere.systemstubs.SystemStubs.withEnvironmentVariable;

import java.util.Arrays;
import java.util.Map;
import java.util.stream.Stream;
import org.junit.jupiter.api.parallel.ResourceAccessMode;
import org.junit.jupiter.api.parallel.ResourceLock;
import org.junit.jupiter.api.parallel.Resources;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;

class StatusLoggerPropertiesUtilDoubleTest {

private static final String[] MATCHING_PROPERTY_NAMES = new String[] {
// System properties for version range `[, 2.10)`
"log4j2.StatusLogger.DateFormat",
// System properties for version range `[2.10, 3)`
"log4j2.statusLoggerDateFormat",
// System properties for version range `[3,)`
"log4j2.StatusLogger.dateFormat",
// Environment variables
"LOG4J_STATUS_LOGGER_DATE_FORMAT"
};

private static final String[] NOT_MATCHING_PROPERTY_NAMES =
new String[] {"log4j2.StatusLogger$DateFormat", "log4j2.StàtusLögger.DateFormat"};

private static final class TestCase {

private final boolean matching;

private final String propertyName;

private final String userProvidedPropertyName;

private TestCase(final boolean matching, final String propertyName, final String userProvidedPropertyName) {
this.matching = matching;
this.propertyName = propertyName;
this.userProvidedPropertyName = userProvidedPropertyName;
}

@Override
public String toString() {
return String.format("`%s` %s `%s`", propertyName, matching ? "==" : "!=", userProvidedPropertyName);
}
}

static Stream<TestCase> testCases() {
return Stream.concat(
testCases(true, MATCHING_PROPERTY_NAMES, MATCHING_PROPERTY_NAMES),
testCases(false, MATCHING_PROPERTY_NAMES, NOT_MATCHING_PROPERTY_NAMES));
}

private static Stream<TestCase> testCases(
final boolean matching, final String[] propertyNames, final String[] userProvidedPropertyNames) {
return Arrays.stream(propertyNames).flatMap(propertyName -> Arrays.stream(userProvidedPropertyNames)
.map(userProvidedPropertyName -> new TestCase(matching, propertyName, userProvidedPropertyName)));
}

@ParameterizedTest
@MethodSource("testCases")
@ResourceLock(value = Resources.SYSTEM_PROPERTIES, mode = ResourceAccessMode.READ_WRITE)
void system_properties_should_work(final TestCase testCase) throws Exception {
restoreSystemProperties(() -> {
final String expectedValue = "foo";
System.setProperty(testCase.propertyName, expectedValue);
verifyProperty(testCase, expectedValue);
});
}

@ParameterizedTest
@MethodSource("testCases")
@ResourceLock(value = Resources.GLOBAL, mode = ResourceAccessMode.READ_WRITE)
void environment_variables_should_work(final TestCase testCase) throws Exception {
final String expectedValue = "bar";
withEnvironmentVariable(testCase.propertyName, expectedValue).execute(() -> {
verifyProperty(testCase, expectedValue);
});
}

private static void verifyProperty(final TestCase testCase, final String expectedValue) {
final Map<String, Object> normalizedProperties = readAllAvailableProperties();
final String actualValue = readProperty(normalizedProperties, testCase.userProvidedPropertyName);
if (testCase.matching) {
assertThat(actualValue).describedAs("" + testCase).isEqualTo(expectedValue);
} else {
assertThat(actualValue).describedAs("" + testCase).isNull();
}
}
}

0 comments on commit af045db

Please sign in to comment.