Update NLogLoggerFactory.cs

[akovac35]

Added double-checked locking to minimize locking performance impact.

[304NotModified]

@snakefoot looks fine to me, agree?

[codecov-io]

Codecov Report

Merging #405 into master will decrease coverage by 0.02%.
The diff coverage is 90%.

@@           Coverage Diff            @@
##           master   #405      +/-   ##
========================================
- Coverage   82.02%    82%   -0.03%     
========================================
  Files          12     12              
  Lines        1174   1178       +4     
  Branches      193    194       +1     
========================================
+ Hits          963    966       +3     
  Misses        141    141              
- Partials       70     71       +1

Impacted Files	Coverage Δ
...og.Extensions.Logging/Logging/NLogLoggerFactory.cs	86.84% <90%> (-1.4%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d78f80d...5e676ab. Read the comment docs.

[snakefoot]

@304NotModified looks fine to me, agree?

Not sure I understand the requirement for this change. @akovac35 Are you actually using this class? and where do you have performance issues?

Instead of racing around in an unsafe container without underwear, then I would probably do one of the following:

• Revert NLogLoggerFactory - Emulate Microsoft LoggerFactory by returning same ILogger instance #380 and change back to new allocation every time
• Change to ConcurrentDictionary
• Change to reader/writer lock

But I'm curious why something that works for Microsoft Extension Logging is not good enough for NLog.Extensions.Logging. So curious where you experience performance issues? (Especially since NLog LoggerFactory is not used by default)

[akovac35]

Well in this case there is simply no need to do any locking for the reading part. If an object already exists, there is no need to lock anything and simply return the object, if it doesn't - then do the locking and check again if the object was inserted just before the lock occurred. The Microsoft's implementation should be fixed as well, there is much needless locking on many places in the overall .NET Core implementation.

So to promote a general practice, this pull request should be excepted.

[snakefoot]

So you just want to make unsafe code because it feels better? If you can convince Microsoft with a merged pull-request for their repository, then I don't mind changing NLogLoggerFactory so it is in sync again with theirs.

[akovac35]

Ok, I will try with them first. I don't see what could be unsafe about the proposed change though. Is this a general concern over design ambiguity originating from MS implementation?

[snakefoot]

Yes the dictionary with lock was introduced to align with Microsoft implementation. Dictionary lookups are not safe when other thread is mutating the collection. Hence why ConcurrentDictionary was invented. But if changes are made to Microsoft LoggerFactory then please tell so we can align.

[akovac35]

You are correct. I will cancel this pull request.