Codecov Report

Merging #3048 into dev will decrease coverage by <1%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##             dev   #3048    +/-   ##
======================================
- Coverage     80%     80%   -<1%     
======================================
  Files        349     349            
  Lines      27549   27549            
  Branches    3654    3654            
======================================
- Hits       21914   21903    -11     
- Misses      4583    4596    +13     
+ Partials    1052    1050     -2

prefer testing the global mutex, before disabling it

I prefer not throwing exceptions as part of normal operation. The ConcurrentWrites is useful with IIS with multiple AppDomains. But on NetCore then there is no longer multiple AppDomains, so there will only be a single NLog instance.

Also the ConcurrentWrites introduces an overhead and complexity, that most NetCore-application do not need. Because WindowsMultiProcessFileAppender is not available on NetCore.

Also notice that ConcurrentWrites already defaults to false when using NetStandard13. Prefer to have the same consistent/stable behavior for all NetCore-platforms.

I prefer not throwing exceptions as part of normal operation.

Me neither, but there isn't a property to check. So I this case I think it's a better option than changing the default for all.

Also note that frameworks like MVC also throw exceptions under the hood in normal operation. Last but not least, we are preventing a exception in this case (the mutex not available exception)

I think it's a better option than changing the default for all.

Well I think it is a better default to have ConcurrentWrites = false for all NetCore-platforms. Mostly because the time of the IIS and multiple AppDomains does not exist on NetCore. And having NetCore-platforms without support mutex, makes a good excuse to disable it by default (along with avoiding the extra complexity and overhead).

And having NetCore-platforms without support mutex, makes a good excuse to disable it by default (along with avoiding the extra complexity and overhead).

We could detect that (try-catch) and that isn't difficult? I don't like to move to default false, if there isn't another option.

IMO smart defaults (detect it) are much better defaults than a black-white choice.

@304NotModified For NLog 5.0 as it is a minor breaking change.

@304NotModified prefer testing the global mutex, before disabling it

Well if you look at #2742 then the attempt of using the mutex takes down the application (UWP NetNative)

Still think that NetCore should have ConcurrentWrites=false by default, as #3079 will only help on certain NetCore platforms. Breaking change for NLog 5.0

It will give the highest compatibility and the best performance on the NetCore-platform. IIS-users will not be affected as they will be using legacy Net45.

IIS-users will not be affected as they will be using legacy Net45.

I'm not sure if that's true

@304NotModified I'm not sure if that's true

Please explain how changing the default for NetCore will affect legacy Net45 applications?

I mean, IIS users aren't limited to non core .NET (I think)

@304NotModified I mean, IIS users aren't limited to non core .NET (I think)

You mean the new In-process hosting model ?

https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/iis/?view=aspnetcore-2.2

Looks like one have to wait for NetCore 3.0 to know the final level of isolation. (Still BETA mode for NetCore2.2)

But until now it has been one process/appdomain for one website on NetCore. For legacy IIS then it is possible to spin up an extra AppDomain for the same website. This extra AppDomain for the same website required across-AppDomain file-synchronization.

I think we should disable it for all platforms, or none.

What is the performance gain when ConcurrentWrites=false?

From #3490 then:

• KeepFileOpen=true, ConcurrentWrites=false = 5,387.220 ns
• KeepFileOpen=true, ConcurrentWrites=true = 11,171.048 ns

But I guess this is with atomic-file-appender, and not with forceManaged=true. I'm guessing it will be like this:

• KeepFileOpen=true, ConcurrentWrites=true, ForceMutexConcurrentWrites=true = 20,171.048 ns

So for NetCore then ConcurrentWrites=false will be 4 times faster and more compatible from the start.

I think we should disable it for all platforms, or none.

;)

From #3490 then:

I think it's more important to compare when KeepFileOpen=false (the default).

I think there are 3 audiences for NLog.

1. The ones that just use NLog. It should be simple and convenient . Performance is important, but perfect is not needed.
2. The ones that needs performance. They should have the controls (and good written advice) to get the best performance - a (small) feature loss is acceptable.
3. Benchmarks. Those are benchmarking unrealistic things. Nobody does while(true) {doLog} in real environments (I hope ;)).

I think it's more important to to focus on 1 and 2. If we focus on 3, that could hurt 1 and 2. That we will lose on 3 is really pity (I've seen comparisons before) - but that is an explicit choice.

I think it's more important to compare when KeepFileOpen=false (the default).

When using KeepFileOpen=false then it doesn't change performance with ConcurrentWrites=false. The only difference is that it retries file-open-operation on failure (when conflict with other applications/appdomains). Actually think that KeepFileOpen=false should ignore ConcurrentWrites and always perform retry on failure.

Should rename these parameters:

• concurrentWriteAttempts -> FileOpenRetryCount
• concurrentWriteAttemptDelay -> FileOpenRetryDelay

And change this line:

Into:

if ((!CreateFileParameters.ConcurrentWrites && !CreateFileParameters.KeepFileOpen) || i + 1 == CreateFileParameters.FileOpenRetryCount)

My priority is this:

• Compatibility with most platforms
• Performance

So I don't mind that KeepFileOpen=false by default (Though not a great default for performance). But I do mind that ConcurrentWrites=true is enabled by default as it hurts performance and give compatibility issues with KeepFileOpen=true.

so could we set ConcurrentWrites to false when unset and KeepFileOpen=true?

(sorry if this is proposed before)

so could we set ConcurrentWrites to false when unset and KeepFileOpen=true?

Same as having ConcurrentWrites=false by default. And perform the above rename of parameters (FileOpenRetryCount + FileOpenRetryDelay) so they apply for both ConcurrentWrites=true and KeepFileOpen=false.

When using KeepFileOpen=false with ConcurrentWrites=true with archiving enabled, then it will create the archive mutex.

We should not have the defaults for the "lowest" platform

Then you should also have KeepFileOpen=true as default :). I prefer compatibility over performance.

And if you notice the code in this PR, then I have given KeepFileOpen=false same behavior as if ConcurrentWrites=true (Performs file-open retries, but no mutex).

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

85.7% Coverage
0.0% Duplication

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

85.7% Coverage
0.0% Duplication

Think this belongs to NLog 5.0, since the goal is to make NLog to be more slim and work on more platforms out-of-the-box (UWP and Xamarin Platforms without access to global operation resources like mutex, as they live in sandbox).

Especially because NLog 5.0 drops Xamarin Android an iOS platforms (that never used global mutex before), and will most likely fail when using NetStandard FileTarget unless ConcurrentWrites is disabled.

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

100.0% Coverage
0.0% Duplication

Updated wiki: https://github.com/NLog/NLog/wiki/File-target