Reconsider exception throwing policy

[D4N]

tl;dr; Imho we throw too many exceptions and should instead make the code more robust.

Since I have started contributing to exiv2, I have seen a steady increase in the amount of exceptions that we throw from the code (mainly as quick fixes to circumvent issues in fuzzed files) and I am unsure whether that is such a good thing. To illustrate what I mean, take a look at #180, especially at the changes to the test suite. A lot of the fuzzed files that were causing exceptions in the pre-#180 code were now no longer throwing exceptions and instead parsed the fuzzed file (kind-of ok) and instead printed warnings to stderr.

I am afraid, that if we continue the current trend of throwing exceptions for every problem that we find, then we'll be in trouble in the future: The code will be harder to use by users of the library, as they'll have to catch exceptions and find ways how to report them to the user (which is quite hard for generic errors like kerCorruptedMetadata). Also, I have the feeling, that we are currently using exceptions as a "short-cut": instead of fixing the code to be more robust, we just throw an exception and the thing is "fixed". However, that's tactical programming and a bad idea in the long term (unless we want more technical debt).

Therefore I propose the following policy for exceptions for discussion:

• The code should be as robust as possible and assume sane defaults for parsed values, when the retrieved value is obviously wrong. E.g. if the file ends prematurely, the code should parse as much as possible and return everything that appeared to be valid and not throw an exception at the end.
• Exceptions should be only thrown in truly exceptional circumstances, i.e.: only as a last resort to prevent a crash (e.g. if an array access would read out of bounds) or when there is no way to recover (like when a file cannot be opened).

This has imho the following advantages:

• Reduced cognitive load for downstream library users. They'll know that Exiv2 will only throw an exception either when there is a bug in the library, or if there is some problem, that they can report to the user (like that the file does not exist).
• The code can become more robust, as it'll assume sane values, when the extracted ones are insane. This can be very beneficial, as we are currently mostly testing "unhappy" paths in the test suite, but don't have a lot of tests for files that are slightly corrupted by buggy software. Since most of our users don't live at HEAD, we won't be seeing reports from them until a RC/release, thereby increasing our workload.
• The library will become a lot more efficient for unhappy paths (see https://ned14.github.io/outcome/faq/#what-kind-of-performance-benefits-will-using-outcome-in-my-code-bring for the performance hit that a thrown exception incurs). The performance hit for the happy paths on the other hand should be manageable.

[clanmills]

This is a thoughtful and interesting topic. I'm up to my neck in other stuff at the moment. Let's discuss this next week. I personally have never liked exceptions and prefer to return a status code from every function. Almost everybody disagrees with me and likes exceptions!

You are quite correct in observing that we've been adding exceptions. All of this started with the fuzzed files. I don't recall adding new error types in years gone by.

[piponazo]

I am one of these guys who @clanmills mentioned: I normally use exceptions in my code, but I try to just use them for truly exceptional situations.

In general I share your worries about the abuse of exceptions, and probably in many cases we can just continue parsing the files and taking default values for the missing fields, or assigning "UNKNOWN" values to them.

However, I do not think we can address an issue like this one for the full code base in one go. IMHO, the only sane way of doing this, is to keep this mindset for the future and apply iterative changes as we refactor some parts of the code.

[D4N]

I am one of these guys who @clanmills mentioned: I normally use exceptions in my code, but I try to just use them for truly exceptional situations.

Just to stress this out, I was not intending to blame anyone! Especially since I have been very much guilty of throwing exceptions left, right and center.

However, I do not think we can address an issue like this one for the full code base in one go. IMHO, the only sane way of doing this, is to keep this mindset for the future and apply iterative changes as we refactor some parts of the code.

I definitely agree, this is not something that can be easily done. The purpose of this issue was merely to ask for everyone's opinion on the matter and potentially to formulate a new policy that we can follow in the future.

[clanmills]

I'm going to close this. I'm going through all open issues and either closing them or assigning them milestone:v1.00. I think the effort required to reach v1.00 is possible with the contributors we have available. However I don't think an open-ended matter deserves to consume our limited resources.