Improve exception handling (make it consistent and clear)

[tetrodoxin]

and refactored respective occurrences.

It's the first shot and definitely object to discuss, but I had to start somewhere.

[tetrodoxin]

How can I actually start the tests from command line? Since the VS test runner creates bin/ and obj/ directories which bother source code tests.

[tetrodoxin]

Well, I nearly expected something like this, when I left out the configuration exception. So... back to scratch.

[304NotModified]

Looks really good! 2 things I'm thinking we maybe should handle :

• throw new Exception maybe a NLog specific one?
• throw if throw exceptions is false.

[304NotModified]

Msbuild NLog.proj /t:runtests? (now on mobile)

[304NotModified]

Maybe you can pass the expected exception as type argument?

Eg

HandleException<TException > where TException = class, new()

PS still on mobile. Ignore the syntax errors :p

[tetrodoxin]

So, I'm sort of back after some time at work.
I also thought about using generics with new(), but this way you have absolutely no chance to provide any error message text or inner exception, so the exception thrown would have the correct type, but no further content, which I considered a bad deal.
A factory delegate would be possible too, but I'm not sure if
ex.HandleException(x => new NLogConfigurationException("Error in...", x))
really would be a real benefit.

[tetrodoxin]

An idea I head was something like a wrapper NLogWrapperException<TException> which possibly also could be casted to its inner exception (TException) and then extend XUnit's Assert.Throws<TException>() to check for TException and for NLogWrapperException<TException>, so testing would not be a real additional effort.

[304NotModified]

I also thought about using generics with new(), but this way you have absolutely no chance to provide any error message text or inner exception, so the exception thrown would have the correct type, but no further content, which I considered a bad deal.

Yes you're right! I forgot that Message property is readonly for an Exception.

An idea I head was something like a wrapper NLogWrapperException which possibly also could be casted to its inner exception (TException)

Not sure if this is nice, but...

XUnit's Assert.Throws() to check for TException and for NLogWrapperException, so testing would not be a real additional effort.

This is a nice idea! No need for a cast I think, we can create a ShouldThrows() in NLogTestBase.

edit: this would be a breaking change. Still thinking if we can fix this otherwise....

[304NotModified]

A factory delegate would be possible too, but I'm not sure if
ex.HandleException(x => new NLogConfigurationException("Error in...", x))
really would be a real benefit.

Or maybe something like:ex.HandException<TException> where TException : IExceptionFactory

and

class ExceptionFactory
{
 static TException  Create<TException > (string message, Exception innermessage)
....

}

[304NotModified]

Thinking of this from scratch:

bool shouldThrow = ex.LogException();
if(shouldThrow) throw;

but then we're almost back... (MustBeRethrown is only replaced by LogException)

[tetrodoxin]

It seems it would be a breaking change in any way we consider it useful. So spare it for later? Just fix the problematic occurences?

[304NotModified]

It seems it would be a breaking change in any way we consider it useful.

Well not if we extend/rename the MustBeRethrown?

We get really soon complains if introduce a breaking change ;)

[tetrodoxin]

If we just extend it the way I tried, we will break some tests that rely on specific exceptions to be thrown, which were the reason for me thinking about adjusting/extending the tests by accepting this wrapper exception. Since we cannot properly rethrow a catched exception outside the catch-block, we're stuck between redesign (breaking change) and very little changes.

[304NotModified]

We're stuck between redesign (breaking change) and very little changes.

Agree with this. I'm doubting if there will be a huge benefit for the breaking change (compared to the little change) - yes it looks better, but every change in an unit test will result in changes needed by our users. So if we can fix it without breaking change, that's preferred, despite I really liked our approach.

[tetrodoxin]

To summarize, by now we are at something like this:

if(ex.LogAndCheckRethrow()) throw;

aren't we? LogAndCheckRethrow() extension method would do right this, the logging and the check, if rethrowing is necessary. This would at least not really break any current behavior but ensure the way it was originally designed, rethrow the exception when rethrowExceptions=true and therefore satisfy issue #277
Tenminste iets, hè?

[304NotModified]

Yes, I think that is the best option.

Sehr schön! :p

[304NotModified]

Need some help on this @tetrodoxin ?

[tetrodoxin]

I'm abroad these days and seldom online, will be back next week, at the latest.

-------- Original Message --------
From: Julian Verdurmen notifications@github.com
Sent: October 11, 2015 7:36:17 PM CEST
To: NLog/NLog NLog@noreply.github.com
Cc: "Andreas H." github@interlook.de
Subject: Re: [NLog] added HandleException() extension method (#915)

Need some help on this @tetrodoxin ?

---

Reply to this email directly or view it on GitHub:

#915 (comment)

Andreas Hübner
Sent with Kaiten-Mail on Android.

[304NotModified]

OK thanks for the info!

[codecov-io]

Current coverage is 73.17%

Merging #915 into master will increase coverage by +0.25% as of f631942

@@            master    #915   diff @@
======================================
  Files          263     263       
  Stmts        14875   14838    -37
  Branches      1618    1615     -3
  Methods          0       0       
======================================
+ Hit          10847   10858    +11
+ Partial        416     412     -4
+ Missed        3612    3568    -44

Review entire Coverage Diff as of f631942

---

Uncovered Suggestions

1. +0.10% via ...anceCounterTarget.cs#170...184
2. +0.09% via ...argets/FileTarget.cs#1912...1924
3. +0.08% via ...c/NLog/LogFactory.cs#180...191
4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

[tetrodoxin]

I had to adjust one test class, namely VariableLayoutRendererTests, because it did not fit into the 'expected exception behavior'. It wasn't prepared for exceptions being rethrown, although it was activated in config.

[304NotModified]

Thanks! I checked the code and have some questions:

I had to adjust one test class, namely VariableLayoutRendererTests, because it did not fit into the 'expected exception behavior'. It wasn't prepared for exceptions being rethrown, although it was activated in config.

I don't fully understand this. Should we change the unit tests for this?

• In which cases we don't like to log to the internal log? (exception.MustBeRethrown(false))? I prefer to always log to the internal log?
• In which case we like to call MustRethrowSevere? I think that one should be private? It's a bit unclear when we should use MustRethrowSevere and when MustRethrow. I think I would be nice if it was one method.

[tetrodoxin]

proposal: LogSwallowException.

N.P. Changed that.

proposal: remove unneeded if and combine asignment & declaration of "shallRethrow"
src/NLog/Internal/ExceptionHelper.cs (160)

Oh, I just missed that one. Was a relict of several changes in the code/tries around it.

src/NLog/Common/InternalLogger.cs (341)
please remove "respectExceptions" (not used)

Actually, the mistake was, that respectExceptions wasn't used. I've corrected that.

[tetrodoxin]

src/NLog/Internal/ExceptionHelper.cs
I think it's nice to have whole exception details in the log, so exception.toString()?

proposal: always add the exception to the end of the message. So no need to to pass exception twice (implicit and explicit). Need a lot of changes.

Well, I should have hit on it myself. I changed MustBeRethrown() that way, it always appends exception.ToString() and will fold that behavior into the respective method calls in code by refactoring string formatting.

[tetrodoxin]

loglevel should be warn? (several places)

Yes and no. MustBeRethrown() uses warn level, if the exception is not to be rethrown, otherwise error level. I read the occurences you pointed out so, that they would produce the same behavior, meaning a warning would have been written, if the rethrow hasn't happened.Since no tests were broken, I considered that behavior plausible.

[tetrodoxin]

src/NLog/Config/XmlLoggingConfiguration.cs (331)
is this the same as calling "logsafe"?

I guess you mean LogSwallowExceptions()?
Actually no. Granted, these lines are sort of a bit hacky, but to follow the expected (and test covered) behavior I had to use this combination of calls to MustBeRethrown(). The first one is nearly an equivalent to the former MustRethrowSevere() as it only throws that serious exceptions and does the logging to error level.
The second call is just for encapsulate the check to LogManager.ThrowExceptions to throw a custom exception in that case.

[tetrodoxin]

I must admit, I have no idea why Var_default_after_set_null ever passed without setting ThowExceptions to false, because an NullReferenceException is obvious with the current code in VariableLayoutRenderer.Append()

[tetrodoxin]

Ok, I can see you changed right this code :-)

[304NotModified]

let me know when I can review it again :) Thanks!

[304NotModified]

Yes and no. MustBeRethrown() uses warn level, if the exception is not to be rethrown, otherwise error level.

I was thinking about this. Is Error level not the best way for exceptions? (for all)?

[304NotModified]

I think #1040 broke this PR :(

[304NotModified]

If you added as me collaborator, I can help fixing the last stuff. Just let me know.

[304NotModified]

bit busy on other stuff, will look at this next week

[304NotModified]

superseded by #1117

[304NotModified]

@tetrodoxin I like to have this in 4.3, so I'm busy fixing the conflict and some loose ends.

Thanks for the PR!

[tetrodoxin]

I am really sorry for being absent that last couple of months and I hope, you at least could make some use of the code snippets, we worked out last year.
Cheers!

[304NotModified]

Yes, we did :) This feature has been merged in 4.3

Thanks for the contributions!