ListAppender: Synchronize on list while iterating

[bjmi]

• Prevent ConcurrentModificationException while making snapshots
• Returned snapshots no longer require an unmodifiable view

Checklist

Before we can review and merge your changes, please go through the checklist below. If you're still working on some items, feel free to submit your pull request as a draft—our CI will help guide you through the remaining steps.

✅ Required checks

• License: I confirm that my changes are submitted under the Apache License, Version 2.0.

• Commit signatures: All commits are signed and verifiable. (See GitHub Docs on Commit Signature Verification).

• Code formatting: The code is formatted according to the project’s style guide.

  How to check and fix formatting

  • To check formatting: ./mvnw spotless:check
  • To fix formatting: ./mvnw spotless:apply

  See the build instructions for details.

• Build & Test: I verified that the project builds and all unit tests pass.

  How to build the project

  Run: ./mvnw verify

  See the build instructions for details.

🧪 Tests (select one)

• I have added or updated tests to cover my changes.
• No additional tests are needed for this change.

📝 Changelog (select one)

• I added a changelog entry in src/changelog/.2.x.x. (See Changelog Entry File Guide).
• This is a trivial change and does not require a changelog entry.

[vy]

@bjmi, thanks so much for the contribution. ListAppender is intended for tests, where the programmer has complete control over the intended parallelism, and hence, its thread-safety has never been an issue. Would you mind sharing the rationale of your changes, please? What is the problem you're trying to solve and where/how did you encounter it?

[bjmi]

Thank you for your reply.
We use ListAppender and @LoggerContextSource extensively in our JUnit tests and they work great. We also like the idea of #3832 to get rid of all the unwanted dependencies of log4j-core-test.

Before LOG4J2-2527 the internal Lists events, data and messages were made public accessible via an unmodifiable view.
Since snapshots are now being returned, the unmodifiable wrapper isn't necessary and the caller is free to choose how to proceed with the lists.
For example, the caller isn't forced to create another copy of the copy in order to add entries or remove unwanted ones.
-> unmodifiable wrapper is removed.

I thought the copy constructors of the Collections API use the iterator of the list to be copied. And
synchronizedList(List) clearly states

It is imperative that the user manually synchronize on the returned list when iterating over it:

But it turns out ArrayList(Collection) uses Collection.toArray() to create a copy that doesn't require manual synchronization.
To avoid future surprises esp. with ConcurrentModificationException, making the snapshots is synchronized.
-> as there is no bug at the moment, the additional synchronized blocks can be removed. WDYT?

[vy]

@bjmi, I am really reluctant to "Can you fix this aspect of X for my particular use case, please?" requests, in particular, for ListAppender. It has been touched by several such attempts, and now it is in a pretty inconsistent state:

• JavaDoc states that "This appender is not thread-safe."
• volatile and synchronizedList used arbitrarily to serve for somebody's use case

@bjmi, would you be interested in revamping this tool? What I have in mind is:

1. Simplify and establish complete thread-safety: remove synchronizedList et al. usages and mark all public methods synchronized
2. Improve class JavaDoc
3. Fix countDownLatch JavaDoc
4. Apply trivial IDE suggestions (e.g., for redundant explicit type arguments)
5. Test ListAppender
6. Test ListAppender thread safety (I can provide a simple recipe for this)

[ramanathan1504]

@vy
is there still open can I do complete the above

[vy]

is there still open can I do complete the above

@ramanathan1504, please go ahead!