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

fix: Adding stack trace to panic catch errors #94

Closed
wants to merge 1 commit into from

Conversation

deregtd
Copy link

@deregtd deregtd commented Dec 23, 2024

Pond automatically catches panics without any option to surface them, but the wrapped error doesn't contain stack information so it's nearly impossible to track down the issue as is. This adds a stack trace to the wrapped error generated out of the recover function.

@alitto
Copy link
Owner

alitto commented Dec 24, 2024

Hey @deregtd!
The best way to obtain the panic thrown by a task is to call the Wait() method on the task object returned by the call to pool.Submit(), these are the docs. If the task panics instead of returning an error, the panic is caught and returned wrapped in pond.ErrPanic. This way the error preserves the stack trace.

@deregtd
Copy link
Author

deregtd commented Dec 24, 2024

Forgive my naivete here, but I tried for quite a while to get the stack trace out of the returned error from that, and couldn't find it stored anywhere in the wrapped error, eventually leading me down the road to making this fork to be able to get the stack trace. Other posts like this confirmed my findings. But I could easily be missing something.

@alitto
Copy link
Owner

alitto commented Dec 24, 2024

You are right @deregtd, I did some tests and confirmed the stack trace information is not stored in the error itself so this change can be very helpful.
While reviewing the code I noticed the panic is not properly wrapped when it implements the error interface, so i created a PR with a fix for that which also includes the change to add the stack trace to the error message. Please take a look at it and let me know if that would work for you #96.
Since you came up with this idea, feel free to replicate the changes I made in your own branch so we can merge this PR instead of mine. That way you'll be the author of these changes 😉

@deregtd
Copy link
Author

deregtd commented Dec 24, 2024

Haha I appreciate the offer, but I don't need the recognition. :). Just glad to help and eliminate my fork. :D I'll close this.

@deregtd deregtd closed this Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants