LOG4J2-2899: Fix log4j-1.2-api LogEventWrapper thread data accessors

[carterkozak]

No description provided.

[carterkozak]

@rgoers, I'd appreciate your feedback on this change when you have time. I'm primarily interested in whether or not you think it's reasonable to replace Thread.getAllStackTraces() with the simpler current-thread check, applications with several thousand threads will suffer creating large arrays, and loading many stack traces unnecessarily.

If the cross-thread check is a requirement, I think the following could work:
1: First check the current thread at construction time, if it matches the event thread name there's no more work to do
2: When an identifier or priority is first requested, we can attempt to use ThreadMXBean.getAllThreadIds and use the result to call getThreadInfo with maxDepth zero to avoid forcing a safepoint and reduce the heap requirement of the result.

[rgoers]

Yeah, I don't think it can be guaranteed that the methods will always be called on the same thread. But I am also noticing that the method stores the thread it found but then doesn't returned it if it was cached.

[carterkozak]

I don't think it can be guaranteed that the methods will always be called on the same thread

It can't be guaranteed that the original thread is still around or has the same name either, and it's expensive to look up even if it is (where the cost scales with total live threads). This becomes a question of risk tolerance and computational cost.

I can keep the fallback around, I think the initial current-thread-check will handle most cases, I imagine most log4j1.2 shim users aren't using asynchronous components.
It's probably a good idea to replace the Thread reference with threadId and threadPriority fields to avoid holding thread references if the event sits in a queue for a while.

[rgoers]

Of course it can be guaranteed the original thread is around because most of the time the LogEventWrapper is called it will be from the same thread as the caller. Only when the caller has an AsyncAppender will that change and in that case Log4j’s AsyncAppender will have captured all the LogEvent info. To be honest, I think that code should just be calling event.getThreadName(), event.getThreadId() and event.getThreadPriority().

[carterkozak]

I've updated this to retain the thread-dump fallback, but use the current-thread fast path when the thread names match.

I don't think the log4j 1.2 delegate in this case provides thread ID and priority accessors. My idea to use the thread bean doesn't quite work because the ThreadInfo object doesn't include priority, only the thread ID.

[rgoers]

I'm not sure why you committed this. I was just about to go modify the code to get the information from the event it is wrapping.

[carterkozak]

Sorry Ralph, I didn't know you planned to take it over! Feel free to revert or modify, I'd be happy to review if you like.

[rgoers]

No. I was just going to look into it based on my last comment. But after looking at this I realized it works backwards from what I was thinking (I was thinking of the LogEventAdapter). So the changes you made are appropriate.