-
Notifications
You must be signed in to change notification settings - Fork 145
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
feat: track exceptions in :async activejob adapter #503
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems reasonable to me, assuming it works in a live Rails app. 😁
I left a question about the test harness.
@joshuap I've removed the :sync and the flush block, as I think both of them are not strictly necessary after reading your explanations. I should've looked at these options a bit more when copying the code over :) I've also simplified the test and tested it in a rails app from the console. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking much better! Just one last question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@subzero10 Can I merge this? |
I'm good with merging this 👍 |
Sure go ahead! |
Fixes #361
I have some doubts about this but I wanted to give it a shot.
.notify
correctly and if my usage of.flush
is correct.I am not 100% sure if this would survive a user installing additional callbacks (I will try to add a test for that):async
adapter (I don't think that that's a huge problem as I am not monkeypatching anything)Please have a look.