Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better exception messages #13

Closed
mecha opened this issue Jun 30, 2020 · 3 comments · Fixed by #14
Closed

Better exception messages #13

mecha opened this issue Jun 30, 2020 · 3 comments · Fixed by #14
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@mecha
Copy link
Contributor

mecha commented Jun 30, 2020

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

@XedinUnknown
Copy link
Member

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?

@mecha
Copy link
Contributor Author

mecha commented Jul 6, 2020

Yes, algorithmically trying to deduce which inner exception has the most meaningful message.

@mecha
Copy link
Contributor Author

mecha commented Jul 8, 2020

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 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.

@mecha mecha self-assigned this Jul 8, 2020
mecha added a commit that referenced this issue Jul 8, 2020
@mecha mecha mentioned this issue Jul 8, 2020
@XedinUnknown XedinUnknown added the enhancement New feature or request label Jul 8, 2020
@XedinUnknown XedinUnknown added this to the 0.1 milestone Jul 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants