Fix: ReconfigExistingLoggers sometimes throws an Exception #2228 #2229
Conversation
|
If the problem is caused by the Values-Collection-Enumerator is detecting that the dictionary has been modified while enumerating, then the correct fix is to make the clone under a lock (See alternative solution here: #2131) |
|
I've added a lock around the references to loggerCache. I was hesitant to do this from the start because this is my first time looking at the code and I was concerned about reciprocal effects. |
Codecov Report
@@ Coverage Diff @@
## release-4.4.12 #2229 +/- ##
===============================================
+ Coverage 81% 81% +<1%
===============================================
Files 291 291
Lines 20135 20134 -1
Branches 2394 2394
===============================================
+ Hits 16354 16366 +12
Misses 3152 3152
+ Partials 629 616 -13 |
|
Not sure I understand why you have added a new lock-object. There is already LogFactory.syncRoot which is used all other places before accessing/modifying the loggerCache. This new lock-object doesn't have any effect, besides serializing readers. Why not make the change that I initially suggested here: #2131 ? Could even optimize it, so you change LoggerCache.Loggers-property to return an |
jpdillingham
force-pushed
the
bugfix-2228
branch
from
f18d1dd
to
e0feafb
Compare
|
I see what you mean now. Apologies, I was trying to fix the issue without getting into the weeds. |
|
@jpdillingham Guess you can remove the stale comment:
|
|
Could you rebase this one on branch release-4.4.12? (and change this PR, not sure if you could do that) |
jpdillingham
force-pushed
the
bugfix-2228
branch
from
3a0d0f0
to
624ef73
Compare
|
I believe this will do it. |
src/NLog/LogFactory.cs
Outdated
| @@ -1348,7 +1350,7 @@ private IEnumerable<Logger> GetLoggers() | |||
| List<Logger> values = new List<Logger>(loggerCache.Count); | |||
|
|
|||
| //new list for prevent InvalidOperationException on Travis/Mono. | |||
There was a problem hiding this comment.
this is also an old comment?
There was a problem hiding this comment.
I believe that is irrelevant now due to the upstream lock.
There was a problem hiding this comment.
@jpdillingham If removing the comment, then you should also remove the ToList()-operation.
|
Just to follow up, I've made all of the requested changes and this should be ready to merge whenever you're ready. |
|
Thanks for the reminder. @snakefoot this change is also OK for you isn't? |
|
Yes it looks good to me
…Sent from my Samsung device
-------- Original message --------
From: Julian Verdurmen <notifications@github.com>
Date: 08/08/2017 09:07 (GMT+01:00)
To: NLog/NLog <NLog@noreply.github.com>
Cc: Rolf Kristensen <sweaty1@hotmail.com>, Mention <mention@noreply.github.com>
Subject: Re: [NLog/NLog] Fix: ReconfigExistingLoggers sometimes throws an Exception #2228 (#2229)
Thanks for the reminder. @snakefoot<https://github.com/snakefoot> this change is also OK for you isn't?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#2229 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AK-fnMSCHTDJ4tVBBVSTXvYZVshN1D5Yks5sWAlFgaJpZM4OnZgp>.
|
|
Online! https://www.nuget.org/packages/NLog/4.4.12 Thanks @jpdillingham ! |
|
I just got this same exception from ReconfigExistingLoggers and I am 100% sure i'm on 4.4.12. |
|
Please create a new issue, including stacktrace. Thx! |
Fixes #2228