Skip to content
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

ConcurrentModificationException in getLogs() #21

Closed
AndreyNudko opened this issue Nov 12, 2021 · 7 comments · Fixed by #22
Closed

ConcurrentModificationException in getLogs() #21

AndreyNudko opened this issue Nov 12, 2021 · 7 comments · Fixed by #22

Comments

@AndreyNudko
Copy link
Contributor

We tried LogCaptor in some integration-style tests, where logging actually happens in background thread.
Occasionally getLogs() throws ConcurrentModificationException -
quick look into code shows that ListAppender.list is not thread-safe, so it's almost certainly a race between test and application threads. I think it's not a problem for actual logging since AppenderBase::doAppend is synchronized, but it is a problem when scanning the list.
One possible fix would be to wrap all access to list into synchronized(listAppender). Or, maybe even better, swap it for CopyOnWrite / Collections::synchronizedList.
Any views on that? Happy to submit PR as well.

@Hakky54
Copy link
Owner

Hakky54 commented Nov 12, 2021

Hi Andrey

Thank you for mentioning this issue. It looks like it is indeed not thread safe. I think both suggestions sounds as a good solution and I am ok with you creating and submitting a pull request. Looking forward to it!

@Hakky54
Copy link
Owner

Hakky54 commented Nov 13, 2021

I just tried it out it it looks like synchronize will do the trick, just like you suggested by wrapping it with synchronized(listAppender) before accessing it. So I don't think synchronizedList is needed with this approach. What do you think?

@AndreyNudko
Copy link
Contributor Author

Thanks for taking a look!
I was thinking using synchronizedList would be a slightly cleaner approach - it's contract clearly states that It is imperative that the user manually synchronize on the returned list when traversing it via Iterator, Spliterator or Stream. Synchronising on appender relies on implementation details of the base class, which are sort of explained in http://logback.qos.ch/manual/appenders.html#AppenderBase, but in less strict terms.
That said, I don't think it's a big deal - either approach will work.
I'll try to find some time to do PR in the next couple of days, unless you're going to make changes.

@Hakky54
Copy link
Owner

Hakky54 commented Nov 15, 2021

Sounds good, I am looking forward to your PR with the synchronizedList

AndreyNudko pushed a commit to AndreyNudko/log-captor that referenced this issue Nov 17, 2021
This makes LogCaptor suitable for using in multi-threaded 'integration' tests
@Hakky54 Hakky54 linked a pull request Nov 17, 2021 that will close this issue
Hakky54 pushed a commit that referenced this issue Nov 17, 2021
* #21 Use synchronizedList to capture log events

This makes LogCaptor suitable for using in multi-threaded 'integration' tests

* Return unmodifiable list

Co-authored-by: Andrey Nudko <andrey.nudko>
@Hakky54
Copy link
Owner

Hakky54 commented Nov 17, 2021

@AndreyNudko I will publish this version in the coming days, I will keep you updated

@AndreyNudko
Copy link
Contributor Author

@Hakky54 Awesome, thanks for the quick turnaround!

@Hakky54
Copy link
Owner

Hakky54 commented Nov 17, 2021

@AndreyNudko The latest changes are now available within version 2.7.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants