Skip to content

Improve implementations of LogEvent.toImmutable() and ReusableMessage.memento() #3171

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

Merged
merged 6 commits into from
May 16, 2025

Conversation

ppkarwasz
Copy link
Contributor

The implementations of LogEvent.toImmutable() either use serialization or one of the Log4jLogEvent.Builder constructors and custom per class code.

This PR:

  • Redirects all implementation of LogEvent.toImmutable to the Builder(LogEvent) constructor (except Log4jLogEvent.toImmutable())
  • Improve the constructor to create really immutable and thread-safe instances.
  • Removes the usage of ThrowableProxy for purposes not related to serialization.
  • Fixes some implementations of ReusableMessage.memento().
  • Uses ReusableMessage.memento() instead of ImmutableMessage where applicable.

The implementations of `LogEvent.toImmutable()` either use serialization or one of the `Log4jLogEvent.Builder` constructors and custom per class code.

This PR:

- Redirects all implementation of `LogEvent.toImmutable` to the `Builder(LogEvent)` constructor (except `Log4jLogEvent.toImmutable()`)
- Improve the constructor to create really immutable and thread-safe instances.
- Removes the usage of `ThrowableProxy` for purposes not related to serialization.
- Fixes some implementations of `ReusableMessage.memento()`.
- Uses `ReusableMessage.memento()` instead of `ImmutableMessage` where
  applicable.
@ppkarwasz ppkarwasz self-assigned this Nov 6, 2024
@ppkarwasz ppkarwasz requested a review from vy November 27, 2024 08:58
@ppkarwasz ppkarwasz added this to the 2.25.0 milestone Apr 13, 2025
Adds comments to explain the side effects of the `getFormattedMessage` calls.
@ppkarwasz ppkarwasz requested a review from Copilot May 16, 2025 11:45
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR improves the implementations of LogEvent.toImmutable() and ReusableMessage.memento() by unifying the immutable conversion through the Log4jLogEvent.Builder, removing deprecated and unnecessary thrownProxy usages, and ensuring that messages are properly cached in their immutable forms.

  • Redirects all implementations of LogEvent.toImmutable() to use the Builder(LogEvent) constructor
  • Updates memento() implementations in reusable messages to cache their formatted strings
  • Removes thrownProxy usage from non‐serialization contexts and adapts tests accordingly

Reviewed Changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
log4j-core/impl/ReusableLogEventFactory.java Updates in Javadoc and thread info retrieval to clarify message mutation semantics.
log4j-core/impl/MutableLogEvent.java Replaces createMemento() with toImmutable() and removes thrownProxy references.
log4j-core/impl/Log4jLogEvent.java Refactors immutability, lazy initialization, and thrownProxy handling; adjusts equals/hashCode.
log4j-core/config/LoggerConfig.java Removes deprecated LogEvent instantiation in property evaluation.
log4j-core/async/* Uses toImmutable() consistently for converting mutable events.
log4j-api/message/* Updates memento() implementations to cache formatted messages.
log4j-1.2-api/rewrite/* Simplifies event rewrite by removing thrownProxy setting.
Test files Update assertions to check for immutable messages instead of legacy memento types.
Comments suppressed due to low confidence (1)

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jLogEvent.java:1091

  • [nitpick] Removing thrownProxy from the equals and hashCode methods alters the equality semantics; please verify that this change does not break any contract or expected behavior.
if (thrownProxy != null ? !thrownProxy.equals(that.thrownProxy) : that.thrownProxy != null) {

@ppkarwasz ppkarwasz mentioned this pull request May 3, 2025
26 tasks
@ppkarwasz ppkarwasz moved this from To triage to In progress in Log4j bug tracker May 16, 2025
@ppkarwasz ppkarwasz moved this from In progress to In review in Log4j bug tracker May 16, 2025
@ppkarwasz ppkarwasz merged commit dc6c53a into 2.x May 16, 2025
12 checks passed
@ppkarwasz ppkarwasz deleted the fix/2.x/throwable-proxy-clean-up branch May 16, 2025 14:11
@github-project-automation github-project-automation bot moved this from In review to Done in Log4j bug tracker May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants