You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "ramanathan1504 (via GitHub)" <gi...@apache.org> on 2026/05/01 16:14:11 UTC
[PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
ramanathan1504 opened a new pull request, #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111#discussion_r3280613218
##########
src/changelog/.2.x.x/3926_revamp_list_appender.xml:
##########
@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="
+ https://logging.apache.org/xml/ns
+ https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+ type="changed">
+ <issue id="3926" link="https://github.com/apache/logging-log4j2/pull/3926"/>
+ <description format="asciidoc">
+ Revamped `ListAppender` for thread-safety and clarity:
+ removed `Collections.synchronizedList` wrappers and replaced with `synchronized` on all public methods,
+ replaced `Awaitility` polling in `getMessages(int, long, TimeUnit)` with `Object#wait` / `notifyAll`,
+ improved class and `countDownLatch` Javadoc,
+ and added unit and concurrency tests.
Review Comment:
No need this much of elaboration. Remember that this will eventually end up in release notes, which are read by human.
```suggestion
Revamp `ListAppender` for thread-safety and clarity.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "ramanathan1504 (via GitHub)" <gi...@apache.org>.
ramanathan1504 commented on code in PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111#discussion_r3177685350
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/appender/ListAppender.java:
##########
@@ -188,36 +179,43 @@ public boolean stop(final long timeout, final TimeUnit timeUnit) {
return true;
}
- public ListAppender clear() {
+ public synchronized ListAppender clear() {
events.clear();
messages.clear();
data.clear();
return this;
}
/** Returns an immutable snapshot of captured log events */
- public List<LogEvent> getEvents() {
- return Collections.<LogEvent>unmodifiableList(new ArrayList<>(events));
+ public synchronized List<LogEvent> getEvents() {
+ return Collections.unmodifiableList(new ArrayList<>(events));
}
/** Returns an immutable snapshot of captured messages */
- public List<String> getMessages() {
- return Collections.<String>unmodifiableList(new ArrayList<>(messages));
+ public synchronized List<String> getMessages() {
+ return Collections.unmodifiableList(new ArrayList<>(messages));
}
/**
* Polls the messages list for it to grow to a given minimum size at most timeout timeUnits and return a copy of
* what we have so far.
*/
- 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);
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111#discussion_r3280644535
##########
src/changelog/.2.x.x/3926_revamp_list_appender.xml:
##########
@@ -0,0 +1,12 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns="https://logging.apache.org/xml/ns"
+ xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+ xsi:schemaLocation="
+ https://logging.apache.org/xml/ns
+ https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+ type="changed">
+ <issue id="3926" link="https://github.com/apache/logging-log4j2/pull/3926"/>
Review Comment:
```suggestion
<issue id="3926" link="https://github.com/apache/logging-log4j2/pull/3926"/>
<issue id="4111" link="https://github.com/apache/logging-log4j2/pull/4111"/>
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111#discussion_r3257881969
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/appender/ListAppender.java:
##########
@@ -188,36 +179,43 @@ public boolean stop(final long timeout, final TimeUnit timeUnit) {
return true;
}
- public ListAppender clear() {
+ public synchronized ListAppender clear() {
events.clear();
messages.clear();
data.clear();
return this;
}
/** Returns an immutable snapshot of captured log events */
- public List<LogEvent> getEvents() {
- return Collections.<LogEvent>unmodifiableList(new ArrayList<>(events));
+ public synchronized List<LogEvent> getEvents() {
+ return Collections.unmodifiableList(new ArrayList<>(events));
}
/** Returns an immutable snapshot of captured messages */
- public List<String> getMessages() {
- return Collections.<String>unmodifiableList(new ArrayList<>(messages));
+ public synchronized List<String> getMessages() {
+ return Collections.unmodifiableList(new ArrayList<>(messages));
}
/**
* Polls the messages list for it to grow to a given minimum size at most timeout timeUnits and return a copy of
* what we have so far.
*/
- 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);
Review Comment:
You've introduced a behavioral change: `waitAtMost` was polling regularly, while the new method blocks for `remainingNanos`, _always_. Am I mistaken? If not, please, revert, and strive to keep your changes as minimal as possible. That is, if you can achieve your goal by touching less LoC without giving up on the quality, always do so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy merged PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111#discussion_r3176901452
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/appender/ListAppender.java:
##########
@@ -188,36 +179,43 @@ public boolean stop(final long timeout, final TimeUnit timeUnit) {
return true;
}
- public ListAppender clear() {
+ public synchronized ListAppender clear() {
events.clear();
messages.clear();
data.clear();
return this;
}
/** Returns an immutable snapshot of captured log events */
- public List<LogEvent> getEvents() {
- return Collections.<LogEvent>unmodifiableList(new ArrayList<>(events));
+ public synchronized List<LogEvent> getEvents() {
+ return Collections.unmodifiableList(new ArrayList<>(events));
}
/** Returns an immutable snapshot of captured messages */
- public List<String> getMessages() {
- return Collections.<String>unmodifiableList(new ArrayList<>(messages));
+ public synchronized List<String> getMessages() {
+ return Collections.unmodifiableList(new ArrayList<>(messages));
}
/**
* Polls the messages list for it to grow to a given minimum size at most timeout timeUnits and return a copy of
* what we have so far.
*/
- 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);
Review Comment:
Why did you replace `waitAtMost`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "ramanathan1504 (via GitHub)" <gi...@apache.org>.
ramanathan1504 commented on code in PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111#discussion_r3266474455
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/appender/ListAppender.java:
##########
@@ -188,36 +179,43 @@ public boolean stop(final long timeout, final TimeUnit timeUnit) {
return true;
}
- public ListAppender clear() {
+ public synchronized ListAppender clear() {
events.clear();
messages.clear();
data.clear();
return this;
}
/** Returns an immutable snapshot of captured log events */
- public List<LogEvent> getEvents() {
- return Collections.<LogEvent>unmodifiableList(new ArrayList<>(events));
+ public synchronized List<LogEvent> getEvents() {
+ return Collections.unmodifiableList(new ArrayList<>(events));
}
/** Returns an immutable snapshot of captured messages */
- public List<String> getMessages() {
- return Collections.<String>unmodifiableList(new ArrayList<>(messages));
+ public synchronized List<String> getMessages() {
+ return Collections.unmodifiableList(new ArrayList<>(messages));
}
/**
* Polls the messages list for it to grow to a given minimum size at most timeout timeUnits and return a copy of
* what we have so far.
*/
- 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);
Review Comment:
You're correct. Replacing the polling loop with remainingNanos blocks the thread for the full duration, which is a regression. I will revert this to the original polling structure and ensure the diff is minimal and non-disruptive. Thanks for the feedback.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Revamped `ListAppender` for thread-safety [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on code in PR #4111:
URL: https://github.com/apache/logging-log4j2/pull/4111#discussion_r3280629691
##########
log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/appender/ListAppender.java:
##########
@@ -131,12 +121,14 @@ public void append(final LogEvent event) {
} else {
write(layout.toByteArray(event));
}
- if (countDownLatch != null) {
- countDownLatch.countDown();
+ notifyAll();
Review Comment:
I doubt if `notifyAll` is necessary here.
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org