Consistent Exception handling v3 #1143

304NotModified · 2016-01-08T22:33:56Z

Summary

Preferred way to use:

exception is added to the message
the message should add some
choose InternalLogger.Warn or InternalLogger.Error`

eg. (most preferred)

InternalLogger.Error(exception, "Cannot stop file watching.");

if (exception.MustBeRethrown())
{
    throw;
}

Also OK, but no custom message in the logs:

if (exception.MustBeRethrown())
{
    throw;
}

Only in special cases! (legacy, very good reaons)

if (exception.MustBeRethrownImmediately())
{
    throw;
}

Never use throw ex

Original post

added overloads with exception parameter, analogues to the normal logger.
MustBeRethrown logs to InternalLogger if the exception is not logged before to the InternalLogger
Warn for NLog config errors, Exception otherwise.

Fixes that:

Exception are not thrown as but were expected
LogManager.ThrowExceptions was not always checked
Exceptions we not always logged to the internallogger.

Fixes #637. Fixes #277

supersedes #915 and #1117

todo

fix after review
check testcase fallbackwrapper + throwEx = false
check after merge RegistryLayoutRenderer: Support for layouts, RegistryView (32, 64 bit) and all root key names (HKCU/HKLM etc) #925
double check all mustBeRethrow cases

because InternalLogger checks MustBeRethrownImmediately and MustBeRethrown logs as fallback

codecov-io · 2016-01-08T23:21:18Z

Current coverage is `73.94%`

Merging #1143 into master will increase coverage by +0.08% as of 444ba50

@@            master   #1143   diff @@
======================================
  Files          265     265       
  Stmts        15230   15297    +67
  Branches      1672    1679     +7
  Methods          0       0       
======================================
+ Hit          11249   11311    +62
- Partial        415     420     +5
  Missed        3566    3566

Review entire Coverage Diff as of 444ba50

Uncovered Suggestions

+0.08% via ...argets/FileTarget.cs#1722...1734
+0.08% via ...gingConfiguration.cs#282...293
+0.08% via ...gingConfiguration.cs#244...255
See 7 more...

Powered by Codecov. Updated on successful CI builds.

no need

304NotModified · 2016-01-09T01:08:50Z

@bhaeussermann Do like to check this again? I didn't changed any plans since our nice discussion :)

bhaeussermann · 2016-01-10T15:29:48Z

src/NLog/Common/InternalLogger.cs

+                if (ex != null)
+                {
+                    ex.MarkedAsLoggedToInternalLogger();
+                    formattedMessage += " Exception: " + ex.ToString();


How about adding the exception string to the StringBuilder below instead?

Good idea, thanks!

but that's OK as this is new in 4.3 and the type is still the same.

304NotModified · 2016-01-25T19:23:59Z

Also,
You asked me before is throwExceptions = true is only for debugging. I confirmed that, but I was wrong. For example the FallbackGroupTarget depends on it :(

I was wrong :) The AsyncContinuation is used for that, not a try-catch, And it seems that the exceptions are always written to AsyncContinuation

304NotModified · 2016-01-26T08:43:58Z

@bhaeussermann

If you like to check this again, thanks! I rechecked every call to MustBeRethrown. I changed some inconsistent test cases as I see them as bugfixes.

bhaeussermann · 2016-01-26T16:43:21Z

src/NLog/Config/Factory.cs

@@ -72,12 +72,13 @@ public void ScanTypes(Type[] types, string prefix)
                }
                catch (Exception exception)
                {
+                    InternalLogger.Error(exception, "Failed to add type '{0}'.", t.FullName);
+
                    if (exception.MustBeRethrown())
                    {
                        throw;
                    }


Must this not change to MustBeRethrownImmediately?

Why do you think that?

I think the only reason would be backwardscomp. I think it's bug that the exception was not thrown when throwExceptions=true

bhaeussermann · 2016-01-26T18:31:52Z

You asked me before is throwExceptions = true is only for debugging. I confirmed that, but I was wrong. For example the FallbackGroupTarget depends on it :(

I was wrong :) The AsyncContinuation is used for that, not a try-catch, And it seems that the exceptions are always written to AsyncContinuation

Ok, so I assume that it is then safe to throw an exception where we did not throw an exception before if ThrowExceptions is set to true. Is that correct?

bhaeussermann · 2016-01-26T18:49:02Z

tests/NLog.UnitTests/GetLoggerTests.cs

        }
-
+
+
        [Fact]
        public void InvalidLoggerConfiguration_ThrowsConfigurationException_IfThrowExceptionsFlagIsSet()


The name should say IsNotSet.

no xUnit 2 there

Consistent Exception handling v3

304NotModified added 13 commits January 8, 2016 22:29

added Exception overloads

9a5bb72

Fixed inconistent usage of [Localizable(false)]

800f966

move exception to separate parameter

e03e7a8

Added MustBeRethrownImmediately, log in MustBeRethrown

054a39f

mark exception as logged to InternalLogger, log only once

168b4d3

handle MustBeRethrownImmediately exceptions

d5da1e1

docs

e4866d5

move writing to InternalLogger before MustBeRethrown

228d34a

because InternalLogger checks MustBeRethrownImmediately and MustBeRethrown logs as fallback

null check

f008460

space added

718de76

fix test

d0d7b3b

Merge branch 'master' into excception-handling-v3

86e3101

fix merge

8d5e0f5

304NotModified added 2 commits January 9, 2016 00:28

doc

4a502ce

Don't rethrow NLogConfigurationException Immediately

8bb1d9a

no need

304NotModified added bug Bug report / Bug fix enhancement Improvement on existing feature labels Jan 9, 2016

304NotModified added this to the 4.3 milestone Jan 9, 2016

304NotModified assigned bhaeussermann Jan 9, 2016

304NotModified added the waiting for review label Jan 9, 2016

bhaeussermann reviewed Jan 10, 2016
View reviewed changes

Fix exceptions (now wrapped)

ce8d4d0

but that's OK as this is new in 4.3 and the type is still the same.

304NotModified added 3 commits January 25, 2016 20:42

Fixes for unit tests

c57db42

improve xUnit usage

e09dcdc

Improve xUnit usage

d1b6547

304NotModified removed the breaking change Breaking API change (different to semantic change) label Jan 25, 2016

304NotModified added 8 commits January 25, 2016 23:12

Fix unit tests

1e60215

Improve xUnit usage

00d766d

Merge branch 'master' into excception-handling-v3

521322f

Improve error handling for RegistryLayoutRenderer

411f44d

docs

9457137

Improvements in error logging

5a01548

fix .net 3.5

d4f5ad3

Fix inconsistent exception

3edfa50

304NotModified assigned bhaeussermann and unassigned 304NotModified Jan 26, 2016

304NotModified mentioned this pull request Jan 26, 2016

NLog Exceptions don't get logged in NLog diagnostics #261

Closed

bhaeussermann reviewed Jan 26, 2016
View reviewed changes

304NotModified added 2 commits January 27, 2016 15:25

Fix unit test name => generalised testcase with [InlineData]

47316e7

Silverlight compatible unit tests

c079af5

no xUnit 2 there

304NotModified added a commit that referenced this pull request Jan 27, 2016

Merge pull request #1143 from NLog/excception-handling-v3

d87bc70

Consistent Exception handling v3

304NotModified merged commit d87bc70 into master Jan 27, 2016

304NotModified deleted the excception-handling-v3 branch February 3, 2016 22:51

304NotModified mentioned this pull request Feb 12, 2016

Control throwing of NLogConfigurationExceptions (LogManager.ThrowConfigExceptions) #1238

Merged

304NotModified mentioned this pull request Mar 15, 2016

Allow configuring type of parameters for the database target (including fix for Oracle) #1194

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistent Exception handling v3 #1143

Consistent Exception handling v3 #1143

304NotModified commented Jan 8, 2016

codecov-io commented Jan 8, 2016

304NotModified commented Jan 9, 2016

bhaeussermann Jan 10, 2016

304NotModified Jan 10, 2016

304NotModified commented Jan 25, 2016

304NotModified commented Jan 26, 2016

bhaeussermann Jan 26, 2016

304NotModified Jan 26, 2016

304NotModified Jan 27, 2016

bhaeussermann commented Jan 26, 2016

bhaeussermann Jan 26, 2016

Consistent Exception handling v3 #1143

Consistent Exception handling v3 #1143

Conversation

304NotModified commented Jan 8, 2016

Summary

Original post

codecov-io commented Jan 8, 2016

Current coverage is 73.94%

Uncovered Suggestions

304NotModified commented Jan 9, 2016

bhaeussermann Jan 10, 2016

Choose a reason for hiding this comment

304NotModified Jan 10, 2016

Choose a reason for hiding this comment

304NotModified commented Jan 25, 2016

304NotModified commented Jan 26, 2016

bhaeussermann Jan 26, 2016

Choose a reason for hiding this comment

304NotModified Jan 26, 2016

Choose a reason for hiding this comment

304NotModified Jan 27, 2016

Choose a reason for hiding this comment

bhaeussermann commented Jan 26, 2016

bhaeussermann Jan 26, 2016

Choose a reason for hiding this comment

Current coverage is `73.94%`