You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "bjmi (via GitHub)" <gi...@apache.org> on 2025/09/09 13:05:41 UTC
[PR] ListApender: Synchronize on list while iterating [logging-log4j2]
bjmi opened a new pull request, #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926
* 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
- [X ] **License**: I confirm that my changes are submitted under the [Apache License, Version 2.0](https://apache.org/licenses/LICENSE-2.0).
- [X] **Commit signatures**: All commits are signed and verifiable. (See [GitHub Docs on Commit Signature Verification](https://docs.github.com/en/authentication/managing-commit-signature-verification/about-commit-signature-verification)).
- [X] **Code formatting**: The code is formatted according to the project’s style guide.
<details>
<summary>How to check and fix formatting</summary>
- To **check** formatting: `./mvnw spotless:check`
- To **fix** formatting: `./mvnw spotless:apply`
See [the build instructions](https://logging.apache.org/log4j/2.x/development.html#building) for details.
</details>
- [X] **Build & Test**: I verified that the project builds and all unit tests pass.
<details>
<summary>How to build the project</summary>
Run: `./mvnw verify`
See [the build instructions](https://logging.apache.org/log4j/2.x/development.html#building) for details.
</details>
### 🧪 Tests (select one)
- [ ] I have added or updated tests to cover my changes.
- [X] 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](https://logging.apache.org/log4j/tools/log4j-changelog.html#changelog-entry-file)).
- [X] This is a trivial change and does not require a changelog entry.
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "ramanathan1504 (via GitHub)" <gi...@apache.org>.
ramanathan1504 commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-4302948791
@vy Thanks I will work around this and submit a new PR for issue #3832.
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-3415534711
> is there still open can I do complete the above
@ramanathan1504, please go ahead!
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "bjmi (via GitHub)" <gi...@apache.org>.
bjmi commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-3314609844
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`.
1.
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.
2.
I thought the copy constructors of the Collections API use the iterator of the list to be copied. And
[synchronizedList(List)](https://docs.oracle.com/javase/8/docs/api/java/util/Collections.html#synchronizedList-java.util.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?
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "ramanathan1504 (via GitHub)" <gi...@apache.org>.
ramanathan1504 commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-4291181516
Hi @vy,
Apologies for the long delay here. I’d like to take this forward and implement the `ListAppender` revamp based on your earlier suggestions.
My plan is to:
1. Establish consistent thread-safety using method-level synchronization, removing the current mix of `volatile` and `synchronizedList`.
2. Ensure `append()` stores `event.toImmutable()` to safely support garbage-free logging.
3. Update and clarify the Javadocs (including `countDownLatch` behavior).
4. Apply general code cleanups (diamond operators, `final` fields, etc.).
5. Add functional and concurrency tests to validate correctness and thread-safety.
You mentioned having a simple recipe for testing thread safety—could you please share that so I can align the test suite with your expectations?
Thanks!
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "ramanathan1504 (via GitHub)" <gi...@apache.org>.
ramanathan1504 commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-4291128870
Hi @vy,
Apologies for the long delay in getting back to this. I have been reviewing the requirements and I would like to proceed with the revamp of `ListAppender` as you suggested.
My plan is to:
1. **Establish complete thread-safety** using method-level synchronization (and removing the current mix of `volatile`/`synchronizedList`).
2. **Ensure `append()` captures `toImmutable()` events** to support garbage-free logging.
3. **Update the Javadocs** for the class and the `countDownLatch` functionality.
4. **Clean up the code** (apply diamond operators, final fields, and other IDE suggestions).
5. **Add comprehensive tests**, including a concurrency suite.
You mentioned having a **simple recipe for testing the thread safety**—could you please share that? I want to make sure the test suite meets your expectations.
Thanks!
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy closed pull request #3926: ListAppender: Synchronize on list while iterating
URL: https://github.com/apache/logging-log4j2/pull/3926
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-3308704592
@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?
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-3337437956
@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)
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "ramanathan1504 (via GitHub)" <gi...@apache.org>.
ramanathan1504 commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-3410598947
@vy
is there still open can I do complete the above
--
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] ListAppender: Synchronize on list while iterating [logging-log4j2]
Posted by "vy (via GitHub)" <gi...@apache.org>.
vy commented on PR #3926:
URL: https://github.com/apache/logging-log4j2/pull/3926#issuecomment-4302764813
> You mentioned having a simple recipe for testing thread safety—could you please share that so I can align the test suite with your expectations?
@ramanathan1504, hammer `ListAppender::append` concurrently using 10 workers each trying to append 1000 [deterministic] events, verify that the last state of `ListAppender::getEvents` & `::getMessages` are consistent, annotate the test method with `@RepeatedTest(10)`.
--
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