Do not throw System.Exception

nservicebus

(Francesc Castells) #1

NServiceBus 7.0.0-rc0001

I’ve just encountered the following exception:

System.Exception: ‘Cannot configure routing for assembly XXXX because it contains no types considered as messages. Message types have to either implement NServiceBus.IMessage interface or match a defined message convention.’

Regardless of what caused the exception, which I am investigating, I find it not adequate that NSB throws System.Exception instead of correctly defined exceptions, like NServiceBus.CannotConfigureRoutingException or similar.


(Marc Selis) #2

+1
And same for “No license found”.

As a developer I always configure my debugger to break on all exceptions when they are thrown and disable the exception that I don’t want to see. Ignoring System.Exception hides all other exceptions.

When starting up NSB throws 12 System.Exceptions with the message “no license found” or someting that are all caught by the next layer until it finds the license I configured in the config file. This is very annoying…


(Daniel Marbach) #3

Hi Francesc and Marc

Thanks for providing us with the feedback. By reading through your input, I can identify two problems:

  • Usage of general purpose exceptions over specific exception types which makes it hard to filter out exceptions not interested in when debugging the application
  • Control flow by exception in licensing which makes the debugger halt unnecessarily when break on all exceptions is enabled

would that be a correct summary of the problems you identified?

Regards
Daniel


(Francesc Castells) #4

Hi Daniel,
Regarding the specific exception types. Yes, that’s correct. I would also add that it’s not only for debugging purposes, but also for exception handling in code. Some exceptions need to be handled deeper in the code so some specific handling code can be executed, whether other exceptions need to bubble up and even make the application crash. Right now, this could only be done by parsing the message of the exception.


(Andreas Öhlund) #5

Some exceptions need to be handled deeper in the code so some specific handling code can be executed

Can you elaborate on the use case you had in mind for catching and dealing with the mentioned exception?

System.Exception: ‘Cannot configure routing for assembly XXXX because it contains no types considered as messages. Message types have to either implement NServiceBus.IMessage interface or match a defined message convention.’


(Francesc Castells) #6

Note that my suggestion of using custom exceptions and how to handle them is a general .NET best practice, not really related to that specific exception. In any case, the first thought I had when I saw the exception was that if NSB was complaining for something that I didn’t care (i.e. an endpoint that doesn’t have messages defined yet), I should be able to catch and ignore this exception. But this wasn’t the case, as my problem was that a new convention had an issue and it wasn’t detecting the messages. So, this is not a burning issue for me. I still thing that it should be addressed though.


(Andreas Öhlund) #7

Thanks for the clarification @fcastells, we’re not against providing custom exceptions where it makes sense to catch and handle them so please let us know if you come across other use cases this is applicable.

One example is the MessageDeserializationException thrown should the incoming message fail to deserialize

Cheers,

Andreas


(Daniel Marbach) #8

Hi Francesc,

Following the MS guidelines in https://docs.microsoft.com/en-us/dotnet/standard/design-guidelines/using-standard-exception-types you can indeed say that throwing a general purpose exception for the routing problem you described is not ideal.

For me, that does not necessarily mean automatically for each failure we need to raise a specific custom exception. A custom exception only makes sense IMHO if a particular edge case cannot be expressed meaningfully with one of the standard exceptions available in the .NET Framework. Furthermore, a standard exception might make sense if some instances need to be distinguished from each other and we as framework authors think those cases need to be handled differently. Going back to the routing problem you described it would be entirely valid to throw an InvalidOperationException because the routing configuration is in an invalid state. This would then address your concern but not opt-in for all custom exceptions.

Any thoughts on that?

Regards
Daniel


(Francesc Castells) #9

Hi Daniel,
I think it’s fine to throw ArgumentException, ArgumentNullException and similar, as they do express exactly what the problem is. But I find InvalidOperationException already to broad. In practice you could say that most of the exceptions could be expressed as an InvalidOperationException, for example, sending a null argument is also an invalid operation. I think a library like NSB should try to be as specific as possible always and default to NServiceBusException if a more specific exception really cannot be done (and all specific exceptions should derive from that one).

Just being able to catch NServiceBusException or search it in the logs can be a great help in many situations.
Also, consider that this practice, once used to it, generates a very small overhead for developers. It can also be unit tested easily.


(Simon Cropp) #10

@fcastells the reason behind “not creating exception type for all these scenarios” is that nservicebus explicitly does not want you to catch the exception using code. @MarcS nor does it want you to ignore when “debugger to break on all exceptions”.

Take the “Cannot configure routing for assembly” example. there is no useful code that can be written in a catch (CannotConfigureRoutingException) other than logging the information and stop the process, which you should be doing for Exception anyway.

I realise this is not “best practice”. but all the guidance on best practice is “use custom exceptions so consumers can catch the exception”. if there is not a use case for when the consumer should catch that exception (and as mentioned in many cases should not), then the best practice has not adequately covered all the scenarios


(Simon Cropp) #11

to expand on the above.

From How to Design Exception Hierarchies by Krzysztof Cwalina co-author of the original .net Framework Design Guidelines

Usage errors need to be communicated to the human developer calling the routine. The exception type is not the best way to communicate errors to humans. The message string is a much better way. Therefore, for exceptions that are thrown as a result of usage errors, I would concentrate on designing a really good (that is explanatory) exception message and using one of the existing .NET Framework exception types: ArgumentNullException, ArgumentException, InvalidOperationException, NotSupportedException, etc. In other words, don’t create new exception types for usage errors because the type of the exception does not matter for usage errors. The .NET Framework already has enough types (actually too many, IMHO) to represent every usage error I can think of.


(Marc Selis) #12

I can understand your arguments, but it is extremely annoying that when starting an endpoint with the debugger breaking on all exceptions you get 12 "system.exception"s in a row, all with the message “no license found” while that license is properly configured in the app.config. If it were a “LicenseNotFoundException” I could configure my debugger to ignore that exception type. Now I have to press F5 12 times until NSB finally detects the license configuration in the config and continues to function normally.


(Brandon Ording) #13

The good news is that we have changed that behavior in NServiceBus 7, so you shouldn’t see those exceptions from the licensing code anymore.


(Marc Selis) #14

Thanks! I’ll have to upgrade my code to V7 then :slight_smile: