Better exception messages
mecha opened this issue · 3 comments
The exception that could be thrown when clearing the cache provides no meaningful explanation as to why the clearing could have failed.
There are a few exceptions that this catch could catch, but most of them will most likely never happen during cache clearing (such as this one since the cache clearing process starts by retrieving keys from the database that do start with the prefix).
Clearing errors will typically arise from an exception thrown in delete(), which itself also catches and re-throws from deleteTransient().
Ideally, the actual error message is not lost when an exception is caught and re-thrown. This puts burden on consumers to iterate through the stack trace in hopes of finding the actual reason for a failure. That process alone is sketchy at best, as the consumer would need to somehow know how long they should recurse into the stack trace in order to obtain a meaningful error message.
Suggestions:
a. Do not catch and re-thrown exceptions wherever possible
b. Re-thrown exceptions re-use the inner exception's message
c. Re-thrown exceptions append the inner exception's message to their own
This puts burden on consumers to iterate through the stack trace in hopes of finding the actual reason for a failure. That process alone is sketchy at best, as the consumer would need to somehow know how long they should recurse into the stack trace in order to obtain a meaningful error message.
You are referring to programmatic access to the inner exception with the meaningful message, right?
Yes, algorithmically trying to deduce which inner exception has the most meaningful message.
I have another case where the exception messages are misleading. This one in particular is causing us a lot of issues.
At seemingly random times, CachePool::set() seems to fail with the exception:
Could not write value for key "..." to cache
This exception is thrown here.
It's pretty evident that this is originating from the setTransient() method, seeing as how it's the only thing inside the try..catch block. The issue comes when we look deeper into that method.
The setTransient() method, can actually throw two different types of exceptions:
- the one it throws itself (
RuntimeException) - the one thrown by the
validateTransientKey()call (RangeException)
The method originally in question, CachePool::set() catches RuntimeException, but will catch both of these (since RangeException extends RuntimeException). And in doing so it re-throws a completely new exception with a completely vague message.
And because we simply report these "soft" non-critical errors as a toast popup to the user, we have no idea which of the two exceptions is actually being thrown.
Expected a PR today :) These exceptions re-throws are getting out of hand.