throwExceptions="true" SqlException is not rethrown

[PetrMachek]

We have NLog configured to write logs to database. When the SqlException is thrown, then it's not propagated to our code even if there is parameter throwExceptions in NLog.config.
Exception is thrown in DatabaseTarget.cs line 488
int result = command.ExecuteNonQuery();

Should it behave in this way?

Thank you :)
Petr

[ghost]

You should receive the exception when throw exceptions are enabled. I'll
look into this

[PetrMachek]

Thank you
I stepped thru that, it goes thru conditions
if (exception.MustBeRethrown()) // DatabaseTarget.cs line 424
and MustBeRethrown is in ExceptionHelper.cs, where is not any condition where is reflected NLogConfig

[304NotModified]

Steps:

• create unit test
• fix if test fails

[tetrodoxin]

It turns out, that 'throwExceptions' sets the global property LogManager.ThrowExceptions. But this property is seldom used, as PetrMachek pointed out, most catch blocks rely solely on the MustBeRethrown() extension method.
So I wonder, if this behavior actually appears at more places.
Maybe extend MustBeRethrown() by including a check of LogManager.ThrowExceptions?

[304NotModified]

Good catch! Yes I remember seeing that before.

I think the exception handling is boilerplate code en difficult to fix. IMO they should be replaced with something like:

void handleException(Action a);
T handleException<T>(Func<T> f);

What do you think?

[tetrodoxin]

What is the second one for? What do you expect an exception handler to return?

[304NotModified]

Its wrapping code with returns something.

For example

int test(n) {
 return handleException (() => 2 / n);
} 

[tetrodoxin]

I think I see, but with this quite new way you would have to refactor a lot of code, don't you? And is it really a benefit over

try
{...}
catch(Exception ex) 
{
  if(ex.MustBeRethrown())
  {
    throw;
  } 
} 

I just ask since it seems like a heavy refactoring.

[304NotModified]

You're right on that. So maybe there is something between editing MustBeRethrown and the handleException approach?

[tetrodoxin]

I guess switching to Roslyn and c#6 is no option? Since it supports exception filters for quite this kind of scenario (not depending to specific CLR, I think)
So you could:

try
{
  // some code block
}
catch (Exception ex) when (ex.MustBeRethrown())
{ throw;}

or maybe even

try
{
  // some code block
}
catch (Exception ex) when (ex.MustBeRethrown(() => {do some logging in any case}))
{ throw;}

So, if not... I counted MustBeRethrown() 65 times in 35 source files. If we do not want to change just the behavior of that extension method, but rather the whole structure of exception handling, we're forced to touch every single occurrence/file, either way. Well, it's feasible and may be clean, since I don't see a real compromise between the two approaches that wouldn't be a rotten one.

option I - cleanup structure

• may be the cleanest solution
• needs refactoring of 35 (of about 340) files
• may conflict with pending changes of unmerged branches

option II - alter MustBeRethrown behavior

• needs nearly no refactoring
• keeps current 'boilerplate exception handling'

Shall we start a voting? 😄

EDIT:
sorry, forget about Roslyn, since it's 'real' .NET only, no SL, no WP

[304NotModified]

Option III - replace .MustBeRethrown with .HandleException:

Replace

try
{...}
catch(Exception ex) 
{
// internallogger usage?
  if (ex.MustBeRethrown())
  {
    throw;
  }
// internallogger usage? 
} 

With

try
{...}
catch(Exception ex) 
{
 ex.HandleException();
} 

This extension will:

• rethrow when needed
• logs to internal logger

This is somewhat between option I and II.

pros/cons

• It's a easier refactor than I (Most of the work can we down with replace all / Resharper)
• There is still some boilerplate code
• The boilerplate code is less than before
• Throwing exceptions in a extension can be an anti-pattern
• +: the try catch is more clear
• -: rethrow the exception somewhere else changes the stacktrace (maybe wrap in another exception?) - also counts for 1 I think.

You're right about Roslyn, It wont work outside desktop .Net, yet.

[tetrodoxin]

I dismissed the concept of an extension method for the exceptions, cause I found that rethrowing from elsewhere and putting different handling behaviours in anonymous methods a too heavy price initially. Obviously you're not as condemning as me about that.
But the internal logging isn't completely homogeneous. Sometimes it's a warning, sometimes an error and it's often specified what exactly went wrong, what isn't known in a general handling method. So you have to specify information about what being logged (as error or warning) in that extension method.
I'll scroll through the sites in code next days, that would habe to be refactored, to see what's needed.
Maybe something like

ex.HandleWithWarning("my param {0} was not equal to {1}", strName,  expectedName);

// so the signatures would be
public static void Handle(this Exception ex)  // with rethrow and standard logging

public static void HandleWithWarning(this Exception ex, string format, params object[] args) // with rethrow and log formatstring as warning

public static void HandleWithError(this Exception ex, string format, params object[] args) // with rethrow and log formatstring as error

I believe to remember, that wrap exception as inner exception into a new one is still better than throw ex;, what discards the stack trace, but I'm not sure and will check that.

[304NotModified]

I dismissed the concept of an extension method for the exceptions, cause I found that rethrowing from elsewhere and putting different handling behaviours in anonymous methods a too heavy price initially.

I think you are talking about option 1? As those uses anonymous methods. Option 3 is just a compiler trick without any performance penalty.

But the internal logging isn't completely homogeneous

I would be nice if we fix that :) Maybe everything should be a Error.

I believe to remember, that wrap exception as inner exception into a new one is still better than throw ex;, what discards the stack trace, but I'm not sure and will check that.

Agree,. Throw ex is evil. And we can't do just throw.

// so the signatures would be

public static void Handle(this Exception ex)  // with rethrow and standard logging
public static void HandleWithWarning(this Exception ex, string format, params object[] args) // with rethrow and log formatstring as warning
public static void HandleWithError(this Exception ex, string format, params object[] args) // with rethrow and log formatstring as error

Or just

public static void HandleException(this Exception ex, LogLevel = LogLevel.Error, string message = null,  params object[] args = null)  // with rethrow and standard logging

But I'm a bit confused? Which option has your vote?

[304NotModified]

PS: related #637

[tetrodoxin]

I think I actually was still collecting facts before coming to a concluding vote.
Meanwhile, after I went through some code, I'd go for refactoring using extension method with standard logging. In code, there's often rethrow before logging. Is that expected behavior? Or should we rather log always and then rethrow if wanted/needed?
Furthermore I'm investigating some special cases, as in AsyncHelpers and if they'd require some special treatment.

Where would you place these extension methods?

[304NotModified]

I think I actually was still collecting facts before coming to a concluding vote.

OK, didn't catch that.

In code, there's often rethrow before logging. Is that expected behavior?

I depends. With the following exceptions we like to stop immediately (so no logging and rethrow)

• StackOverflowException
• OutOfMemoryException

Maybe also ThreadAbortException.

All other we like to log before an exception.

Where would you place these extension methods?

https://github.com/NLog/NLog/blob/master/src/NLog/Internal/ExceptionHelper.cs

[tetrodoxin]

These exceptions are the ones, that are listed in MustBeRethrown, I see. Btw: I thought StackOverflowException cannot actually be catched.

[304NotModified]

Well sometimes it seems
http://stackoverflow.com/a/1599238

it's a good idea to keep to code but also add a comment (that most of the time it will not be catched)