Skip to content

Revamped ListAppender for thread-safety#4111

Open
ramanathan1504 wants to merge 4 commits intoapache:2.xfrom
ramanathan1504:listappender-threadsafety
Open

Revamped ListAppender for thread-safety#4111
ramanathan1504 wants to merge 4 commits intoapache:2.xfrom
ramanathan1504:listappender-threadsafety

Conversation

@ramanathan1504
Copy link
Copy Markdown
Contributor

Fixes #3926

Revamp ListAppender: Thread-Safety, Clarity, and Robust Testing

Summary

  • Refactored ListAppender for consistent thread-safety:
    • Removed all Collections.synchronizedList usage.
    • Marked all public instance methods as synchronized for clear, uniform locking.
  • Improved efficiency and correctness of getMessages(int, long, TimeUnit):
    • Replaced external polling (e.g., Awaitility) with Object#wait/notifyAll for in-JVM notification and reduced CPU usage.
  • Enhanced documentation:
    • Expanded and clarified class-level Javadoc.
    • Improved countDownLatch Javadoc with usage examples and clearer intent.
  • Strengthened test coverage:
    • Added and improved unit and concurrency tests for ListAppender.
    • Introduced stress tests: 10 threads × 1000 events, repeated 10 times, verifying both event/message count and content for loss/duplication.
  • No changes to public API or field visibility; all existing usages remain valid.

Testing

  • All new and existing tests pass, including concurrency and stress scenarios.

bjmi and others added 4 commits September 20, 2025 06:58
* Returned snapshots no longer require an unmodifiable view
- Remove synchronizedList wrappers; replace with plain ArrayLists
- Annotate all public methods as synchronized on `this`
- Replace Awaitility polling in getMessages(int,long,TimeUnit) with wait/notifyAll
- Update class-level JavaDoc to reflect thread-safety guarantee
- Rewrite countDownLatch field JavaDoc with a self-contained example
- Remove redundant explicit generic type arguments (IDE cleanup)
- Add ListAppenderTest: unit tests + @RepeatedTest(10) concurrency tests
  that hammer append with 10 workers x 1000 deterministic events each
  and verify getEvents/getMessages are consistent
Fixes apache#3926
public List<String> getMessages(final int minSize, final long timeout, final TimeUnit timeUnit)
public synchronized List<String> getMessages(final int minSize, final long timeout, final TimeUnit timeUnit)
throws InterruptedException {
Awaitility.waitAtMost(timeout, timeUnit).until(() -> messages.size() >= minSize);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you replace waitAtMost?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We achieved the same wait/notify behavior using only standard Java (no external dependencies). If you prefer, we can revert to the old Awaitility-based approach instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

3 participants