-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
(Closeable)ThreadContext putAll removes entire context #2946
Comments
Another test that shows the same failure is this test, which does not use
And another test, simpler, that fails because now
|
Can you check the latest |
I believe so, the problem with |
Sounds like it should fix this issue. I will test with a snapshot-build tomorrow. |
I can confirm that the latest snapshot build fixes our tests. |
Thanks for the confirmation, I am closing it and adding it to the |
This has NOT been fixed (yet). Not all closeables are closed properly... |
@nielsm5, that is |
Thanks, and no worries, mistakes happen. Luckily we've got tests. |
I see now that perhaps 2 weeks ago there was a case that I did not verify when testing the snapshot. Here are 2 unit tests that show the issue. These unit tests only use JDK apis, JUnit, and Log4J2 api. While in the scope of the inner nested CloseableThreadContext, After the context When I comment out the line With this line in the tests, the In Log4J2 2.23.1 both tests pass.
|
@tnleeuw, could you locally compile that branch (see |
I'm trying but I get weird errors in my project about classes not found, when I change my log4j2 dependency to 2.25.0-SNAPSHOT. I'm not quite sure what's going on, Maven doesn't give me a very sensible error message but IntelliJ complains about not being able to load the annotation class NullMarked from jspecify.
And from the Maven output:
This goes away again as soon as I switch back to any release version of Log4J2. I'm a bit puzzled because git blame seems to indicate that jspecify has been part of the project for a long time already, and is not a transitive dependency of release versions of Log4J2 either. |
@tnleeuw, if I understand it right, you are able to successfully compile Log4j, though using it in your project is failing due to the error you cited. I will further investigate the JSpecify issue. In the meantime, could you add JSpecify dependency in the |
I am a bit puzzled too, since annotations were conceived from the start as optional and compilation should not fail if they are missing. Which JDK are you using to compile your project? There is a known bug in JDK 8 (JDK-8152174) that causes problem if optional annotations are missing. |
Adding jspecify to my own dependencies, tests that previously failed do now succeed. Still running the entire test suite. |
I currently use JDK17.0.12, built by Amazon.
Using Maven 3.9.8 from the Maven wrapper |
@tnleeuw, just to get the most obvious probability out of our way, could you try the following instead, please?
|
Done but that doesn't make any difference. Again using Amazon Coretto JDK 17.0.12 to build Log4J2, and also for my project. |
When I add jspecify as dependency to my project, all LogContext / ThreadContext related tests pass. There is 1 failing test, which is an unexpected newline at the end of a logline. That doesn't look related to anything to do with the ThreadContext but it's something for ourselves to keep in mind after the release of 2.25.0. |
@tnleeuw, thank you so much for taking to time the evaluate the fix! 😍 Much appreciated! 🙇
|
You're welcome!
The code uses a custom TestAppender class and a custom Layout class that extends from So as far as I can determine from our custom log4j code, we do not use any of The relevant bit of initialisation code is: IbisPatternLayout(final Configuration config, final String pattern, final Charset charset, final boolean alwaysWriteExceptions, final boolean disableAnsi, final boolean noConsoleNoAnsi) {
super(config, charset);
try {
final PatternParser parser = PatternLayout.createPatternParser(configuration);
final List<PatternFormatter> list = parser.parse(pattern, alwaysWriteExceptions, disableAnsi, noConsoleNoAnsi);
final PatternFormatter[] formatters = list.toArray(new PatternFormatter[0]);
serializer = new PatternSerializer(formatters);
} catch (final RuntimeException ex) {
throw new IllegalArgumentException("Cannot parse pattern '" + pattern + "'", ex);
}
} |
Could you explain your use-case for the following assertion on the "not-in-closeable" key: ThreadContext.clearMap();
try (final CloseableThreadContext.Instance c = CloseableThreadContext.put("inner", "one")) {
ThreadContext.put("not-in-closeable", "true");
}
assertEquals("true", ThreadContext.get("not-in-closeable")); The |
@ppkarwasz This is not a complete answer to your question but see my comment that I just added to the PR: #3050 (comment) |
@tnleeuw,
|
I didn't even realize |
I hear you, however this is how it's always worked up to version 24. I would expect this behavior to keep working as-is without Log4j2 suddenly introducing non-backwards-compatible/breaking changes. |
@sjoerdtalsma @Marcono1234 If you see this message, I am under the impression that your system also depends on Log4J2 ThreadContext behaviour as it was until Log4J2 2.23.1. Since the Log4J team is trying to understand if we genuinely have need / use-case for that behaviour, I would appreciate to hear what is your use-case and what breaks with the current Log4J 2.24.1 behaviour. |
To be more precise the behavior in What we are discussing here is the future of the Log4j API. @rgoers, plans to introduce a new tool called ScopedContext.where("foo", "bar").run(() -> some lambda); This API will clear all the changes to the context data of the current thread once "some lambda" exits. You will be even allowed to add custom Once that is out, we don't exclude the possibility of deprecating |
If there is a way to get from current scope to outer scope, or root scope, then we can "walk that tree" and put things in the outer scope -- which will eventually be cleared, after the top-level process that runs on the thread exits. I don't know what others in my team think about it. |
Hi, thanks for touching base. Let me try to explain our use-case. The purpose of our framework is propagating a snapshot consisting of one or more threadlocal states to a new thread. The features our framework needs are:
I hope this makes some sense? Feel free to reach out if further questions exist. |
I hadn't investigated in depth why talsma-ict/context-propagation#484 respectively talsma-ict/context-propagation#491 failed; based on the Log4j 2 release notes I had assumed it is related to this issue here and suggested to wait for the fix. But now after having looked a bit into this, it seems the failures might not be related to the issue here. Sorry for the confusion. Instead #2374 seems to have caused a behavior change: In 2.23.1 and before it seems However, I am not sure if that has any noticeable effect on production applications because they most likely have a Log4j 2 provider on their classpath (probably log4j-core), or if not they are not using or don't care if It was just the case that https://github.com/talsma-ict/context-propagation previously only had a log4j-api dependency, but was using |
@sjoerdtalsma @Marcono1234 Thanks for getting back to us! We also do have some code that copies state from one thread to another including the Log4J2 ThreadContext, but at least as of 2.24.1 that part does not break. |
Description
The ThreadContext no longer works as intended. The new version closes/removes items from the stack when it shouldn't.
CloseableThreadContext calls
ThreadContext.putAll
.Once putAll has been called, the stack is gone, and only the newly inserted values are present.
Since it's used by
CloseableThreadContext#close()
it's quite destructive...Configuration
Version: 2.24.0
Operating system: All
JDK: All
Reproduction
and
The text was updated successfully, but these errors were encountered: