-
Notifications
You must be signed in to change notification settings - Fork 147
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
Make sure to close DB connections before fork #511
base: main
Are you sure you want to change the base?
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.
👋 sqlite3-ruby maintainer here. The general approach is sound, I just have a question about the connection pool roles.
self.connection_handler.clear_all_connections!(:writing) | ||
self.connection_handler.clear_all_connections!(:reading) |
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.
I'm curious why two calls are being made for reading and writing roles here.
By default, if no arguments are passed to ConnectionHandler#clear_all_connections!
it should clear all roles. Did you see something unexpected that led you to code it this way?
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.
Hello, to be honest I was just copy pasting from another app without digging the reason.
The behavior for this method changes in rails 7.2 according to this PR. I just checked and solid queue still supports rails 7.1, so that would ensure compatibility.
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.
@alxckn Ah! That's a very good reason to do it that way. Thanks for the explanation! 👍
Thanks @alxckn! Looks like this breaks the tests for SQLite for some Ruby versions (3.2.0 and 3.3.2, no idea why just these), with some quite odd DB-related failures (I hadn't seen these before). Do you have any idea why this might be the case? I suspect this is some artificial consequence of the tests, but I have no idea what 😕 |
@rosa , I don't know why the tests fail either, I'll find time to dig |
@rosa I extended the disconnect to all forks made in the lib, that seems to have taken care of the initial sqlite failures. It then made most mysql tests fail, turned out the change affected this processes_lifecycle_test, the supervisor would deregister and so move the job from claimed to ready before the test could verify it remained claimed. Adjusting the test to sleep a bit less seems to fix it, but probably in a (more?) flaky way. Now there are still failing runs, because of DB deadlocks (ex. sqlite, PG). Looks scary, they seem to go away upon retry. I wonder if that's something that you've already seen happen within this repo, or if it's definitely something that this PR is bringing? |
Fixes #506. This change helps remove the warning issued by sqlite3 gem