Skip to content
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

[LOG4J2-2368] Recursive logging doesn't clobber cached StringBuidlers #189

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@carterkozak
Copy link
Contributor

commented Jul 9, 2018

No description provided.

@@ -114,6 +115,10 @@ public B setHeaderSerializer(final Serializer headerSerializer) {
* @return a {@code StringBuilder}
*/
protected static StringBuilder getStringBuilder() {
if (AbstractLogger.getRecursionDepth() > 1) { // LOG4J2-2368

This comment has been minimized.

Copy link
@garydgregory

garydgregory Jul 9, 2018

Contributor

Curious and somewhat unrelated: Why isn't the recursion depth a Integer or AtomicInteger?

This comment has been minimized.

Copy link
@carterkozak

carterkozak Jul 10, 2018

Author Contributor

My guess is Integer would require boxing/unboxing on set/access and AtomicInteger incurs a cost for thread safety. The array is a cheap MutableInt. @remkop could likely provide a definitive answer.

This comment has been minimized.

Copy link
@remkop

remkop Jul 10, 2018

Contributor

AbstractLogger.recursionDepthHolder is of type ThreadLocal<int[]> because that was the simplest data structure that met all the requirements:

  • It needs to be thread-local, because when multiple threads are logging concurrently each thread has its own recursion depth
  • Incrementing and decrementing the counter happens at least once for each log message so should be fast and not allocate any objects

An alternative would have been to use ThreadLocal<AtomicInteger>. This would allow the AbstractLogger code to look like getRecursionDepthHolder().incrementAndGet() instead of getRecursionDepthHolder()[0]++.

That's perhaps a bit nicer, but I thought it looked strange to hold a thread-safe atomic data structure inside a thread-local. ThreadLocal<int[]> felt simpler.

asfgit pushed a commit that referenced this pull request Jul 10, 2018

@asfgit asfgit closed this in 38e59a1 Jul 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.