ppkarwasz commented on code in PR #2438: URL: https://github.com/apache/logging-log4j2/pull/2438#discussion_r1554551301
########## log4j-api/src/main/java/org/apache/logging/log4j/spi/internal/DefaultScopedContextProvider.java: ########## @@ -0,0 +1,421 @@ +/* + * 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.spi.internal; + +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.function.Supplier; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.ScopedContext; +import org.apache.logging.log4j.ThreadContext; +import org.apache.logging.log4j.spi.ScopedContextProvider; +import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.StringMap; + +/** + * An implementation of {@link ScopedContextProvider} that uses the simplest implementation. + * @since 2.24.0 + */ +public class DefaultScopedContextProvider implements ScopedContextProvider { + + public static final Logger LOGGER = StatusLogger.getLogger(); + public static final ScopedContextProvider INSTANCE = new DefaultScopedContextProvider(); + + private final ThreadLocal<Instance> scopedContext = new ThreadLocal<>(); + + /** + * Returns an immutable Map containing all the key/value pairs as Object objects. + * @return The current context Instance. + */ + private Optional<Instance> getContext() { + return Optional.ofNullable(scopedContext.get()); + } + + /** + * Add the ScopeContext. + * @param context The ScopeContext. + */ + private void addScopedContext(final Instance context) { + Instance current = scopedContext.get(); + scopedContext.set(context); + } + + /** + * Remove the top ScopeContext. + */ + private void removeScopedContext() { + final Instance current = scopedContext.get(); + if (current != null) { + if (current.parent != null) { + scopedContext.set(current.parent); + } else { + scopedContext.remove(); + } + } + } + + @Override + public Map<String, ?> getContextMap() { + final Optional<Instance> context = getContext(); + return context.isPresent() + && context.get().contextMap != null + && !context.get().contextMap.isEmpty() + ? Collections.unmodifiableMap(context.get().contextMap) + : Collections.emptyMap(); + } + + /** + * Return the value of the key from the current ScopedContext, if there is one and the key exists. + * @param key The key. + * @return The value of the key in the current ScopedContext. + */ + @Override + public Object getValue(final String key) { + final Optional<Instance> context = getContext(); + return context.map(instance -> instance.contextMap) + .map(map -> map.get(key)) + .orElse(null); + } + + /** + * Return String value of the key from the current ScopedContext, if there is one and the key exists. + * @param key The key. + * @return The value of the key in the current ScopedContext. + */ + @Override + public String getString(final String key) { + final Optional<Instance> context = getContext(); + if (context.isPresent()) { + final Object obj = context.get().contextMap.get(key); Review Comment: Here you (correctly) assume that `contextMap` is not null, so there will be no NPE. ########## log4j-api/src/main/java/org/apache/logging/log4j/spi/internal/DefaultScopedContextProvider.java: ########## @@ -0,0 +1,421 @@ +/* + * 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.spi.internal; + +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.function.Supplier; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.ScopedContext; +import org.apache.logging.log4j.ThreadContext; +import org.apache.logging.log4j.spi.ScopedContextProvider; +import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.StringMap; + +/** + * An implementation of {@link ScopedContextProvider} that uses the simplest implementation. + * @since 2.24.0 + */ +public class DefaultScopedContextProvider implements ScopedContextProvider { + + public static final Logger LOGGER = StatusLogger.getLogger(); + public static final ScopedContextProvider INSTANCE = new DefaultScopedContextProvider(); + + private final ThreadLocal<Instance> scopedContext = new ThreadLocal<>(); + + /** + * Returns an immutable Map containing all the key/value pairs as Object objects. + * @return The current context Instance. + */ + private Optional<Instance> getContext() { + return Optional.ofNullable(scopedContext.get()); + } + + /** + * Add the ScopeContext. + * @param context The ScopeContext. + */ + private void addScopedContext(final Instance context) { + Instance current = scopedContext.get(); + scopedContext.set(context); + } + + /** + * Remove the top ScopeContext. + */ + private void removeScopedContext() { + final Instance current = scopedContext.get(); + if (current != null) { + if (current.parent != null) { + scopedContext.set(current.parent); + } else { + scopedContext.remove(); + } + } + } + + @Override + public Map<String, ?> getContextMap() { + final Optional<Instance> context = getContext(); + return context.isPresent() + && context.get().contextMap != null + && !context.get().contextMap.isEmpty() + ? Collections.unmodifiableMap(context.get().contextMap) + : Collections.emptyMap(); Review Comment: Due to `setupContext`, the `contextMap` field should be not null here. As for the immutability of the map, we could ensure it in the constructor of `Instance`. This code could be reduced to: ```java context.map(c -> c.contextMap).orElse(Collections.emptyMap()); ``` ########## log4j-api/src/main/java/org/apache/logging/log4j/spi/internal/DefaultScopedContextProvider.java: ########## @@ -0,0 +1,421 @@ +/* + * 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.spi.internal; + +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.Callable; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.function.Supplier; +import org.apache.logging.log4j.Logger; +import org.apache.logging.log4j.ScopedContext; +import org.apache.logging.log4j.ThreadContext; +import org.apache.logging.log4j.spi.ScopedContextProvider; +import org.apache.logging.log4j.status.StatusLogger; +import org.apache.logging.log4j.util.StringMap; + +/** + * An implementation of {@link ScopedContextProvider} that uses the simplest implementation. + * @since 2.24.0 + */ +public class DefaultScopedContextProvider implements ScopedContextProvider { + + public static final Logger LOGGER = StatusLogger.getLogger(); + public static final ScopedContextProvider INSTANCE = new DefaultScopedContextProvider(); + + private final ThreadLocal<Instance> scopedContext = new ThreadLocal<>(); + + /** + * Returns an immutable Map containing all the key/value pairs as Object objects. + * @return The current context Instance. + */ + private Optional<Instance> getContext() { + return Optional.ofNullable(scopedContext.get()); Review Comment: This might allocate an instance of `Optional` at each call. Maybe we could return a constant: ```java private final ScopedContext.Instance EMPTY_INSTANCE = new Instance(this, Collections.emptyMap()); ``` if nothing is attached to the `ThreadLocal`? ########## log4j-api/src/main/java/org/apache/logging/log4j/message/package-info.java: ########## @@ -20,7 +20,7 @@ */ @Export /** - * Bumped to 2.22.0, since FormattedMessage behavior changed. + * Bumped to 2.24.0, since FormattedMessage behavior changede. Review Comment: I have the feeling that `ResourceLogger` should allow multiple implementations that can be changed via configuration. The version in this PR puts the map in the `ScopedContext` as @rocketraman requires, but as you commented yourself, it also makes sense to use a `MapMessage`. I think companies should be able to change the behavior of `ResourceLogger` without changing a single line of code. ########## log4j-api-test/src/test/java/org/apache/logging/log4j/ScopedContextTest.java: ########## @@ -0,0 +1,194 @@ +/* + * 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; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.equalTo; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.junit.jupiter.api.Assertions.fail; + +import java.util.concurrent.ArrayBlockingQueue; +import java.util.concurrent.BlockingQueue; +import java.util.concurrent.ExecutionException; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Future; +import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicLong; +import org.junit.jupiter.api.Test; + +public class ScopedContextTest { + + @Test + public void testScope() { + ScopedContext.where("key1", "Log4j2").run(() -> assertThat(ScopedContext.get("key1"), equalTo("Log4j2"))); + ScopedContext.where("key1", "value1").run(() -> { + assertThat(ScopedContext.get("key1"), equalTo("value1")); + ScopedContext.where("key2", "value2").run(() -> { + assertThat(ScopedContext.get("key1"), equalTo("value1")); + assertThat(ScopedContext.get("key2"), equalTo("value2")); + }); + }); Review Comment: These tests could use `ScopedContextProviderSuite`. The idea behind introducing that class in my PR was as a sort of mini TCK that all implementations must pass. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
