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

Make copy of appender list when removing all, for increased thread-safety #222

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tmct
Copy link

@tmct tmct commented Jan 30, 2025

Hello,

We have been witnessing this error now and again while using log4net 2.0.15:

Collection was modified; enumeration operation may not execute.
   at log4net.Appender.AppenderCollection.Enumerator.MoveNext()
   at log4net.Util.AppenderAttachedImpl.RemoveAllAppenders()
   at log4net.Repository.Hierarchy.Logger.RemoveAllAppenders()
   at log4net.Repository.Hierarchy.Hierarchy.Shutdown()
   at log4net.Core.LoggerManager.Shutdown()
   at log4net.LogManager.Shutdown()

It looks like the code on master is still susceptible to this, what do you think of this change? (The try-catch should be wel-equipped to handle appenders that have left the List, so long as we can still access their name for the error message...)

Thanks,
Tom

@FreeAndNil
Copy link
Contributor

@tmct thanks for your pull request.

I would prefer the pattern already used in AppendLoopOnAppenders():

_appenderArray ??= _appenderList.ToArray();
foreach (IAppender appender in _appenderArray)

Can you adjust the code?

@tmct
Copy link
Author

tmct commented Jan 30, 2025

Yes of course, consistency can be very useful! Thank you.

@tmct
Copy link
Author

tmct commented Jan 30, 2025

Hi @FreeAndNil - I had a look and I don't think the private _appenderArray was actually doing any caching at all beyond the scope of each individual method. And it likely hinders the thread-safeness of anything that uses it. Most likely the original intent was to protect against exactly the thing we're seeing here.

I've removed it everywhere if that's ok?

@FreeAndNil
Copy link
Contributor

@tmct the _appenderArray had the advantage to copy the contents only once into an array.
Now every method call (even those who only read) create a new Array.

I'm currently considering switching to System.ComponentModel.Collections.Collection as base class.
But that would probably be a breaking change.

@tmct
Copy link
Author

tmct commented Jan 31, 2025

Ah yes you could be right, this could specifically have a performance impact in AppendLoopOnAppenders, being the method among these that we expect to be called frequently without a change in the list, the problem being the repeated filing of the same array.

I can address that later today or Monday, thanks.

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 this pull request may close these issues.

2 participants