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

Use original stacktrace on errors that happen in Cachex.fetch! fallbacks #252

Closed
rinpatch opened this issue Sep 9, 2020 · 6 comments · Fixed by #292
Closed

Use original stacktrace on errors that happen in Cachex.fetch! fallbacks #252

rinpatch opened this issue Sep 9, 2020 · 6 comments · Fixed by #292
Assignees
Milestone

Comments

@rinpatch
Copy link

rinpatch commented Sep 9, 2020

Sometimes the fallback function logic can be quite complex and call a lot of functions, when these functions raise unexpectedly raise errors Cachex just returns Cachex.ExecutionError with the error message copied from the exception. I've been in multiple situations when the error message is not enough to identify where the issue happens and you have to check the all executed code manually or comment out the fetch and try to reproduce the issue without caching.

It would be much faster do debug some issues if original stacktrace was available (currently the stacktrace in exceptions cachex throws just ends at Cachex.unwrap_unsafe). I wrote a proof of concept patch that does just that f03beed , here is how it looks:

20200909_18h53m04s_grim

If you think this feature makes sense upstream, I will finish the TODOs and submit a PR.

@rinpatch
Copy link
Author

rinpatch commented Mar 3, 2021

@whitfin can I please get feedback on this? If this is something that would make sense for the project, I will fix the remaining issues and submit a PR

@thbar
Copy link

thbar commented Mar 8, 2021

@rinpatch just wanted to say thanks for this commit, providing good inspiration to me to better handle errors inside our computations functions (e.g. using Ecto, which can raise on its own).

@whitfin
Copy link
Owner

whitfin commented May 25, 2021

@rinpatch this is awesome; sorry to get back to you so late.

If you have anything for this, feel free to throw it in a PR and we can get it included. I'll hold off on cutting v3.4.0 for a while so we can try get this in (no pressure).

@whitfin whitfin added this to the v3.4.0 milestone May 25, 2021
@whitfin whitfin self-assigned this May 25, 2021
@whitfin whitfin removed this from the v3.4.0 milestone Jun 2, 2021
@whitfin whitfin added this to the v3.5.0 milestone May 31, 2022
@whitfin
Copy link
Owner

whitfin commented Jul 10, 2022

I'm gonna land this in v3.5.0 based on the concept in the linked commits. Looks good enough, just a couple of smaller tweaks, then it should be in v3.5.0 when shipped! 🎉

@pdelacroix
Copy link

I'm wondering something... would it be possible to include the whole rescued exception in ExecutionError? this way we could re-raise it in the caller if needed, and we would keep all its fields

@whitfin
Copy link
Owner

whitfin commented Dec 24, 2023

@pdelacroix yes, I'm considering this. It's a bit wasteful, I would rather just remove ExecutionError from this flow completely and raise the original error. That requires a major bump though, so it's on hold for the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants