Enhance Thread and Fiber Safety in ActiveRecordExaminer #353
Conversation
|
@swrobel Would you mind reviewing this PR and provide any feedback or insights? I learned a LOT from following you around in GitHub issues and discussions. |
There was a problem hiding this comment.
The PR summary isn't informative; it reads as if it's machine-generated, it doesn't explain why the changes are necessary or correct. In fact I suspect that they're incorrect or at least inefficient. Could you please explain? I don't understand how that does anything to "improve fiber and thread safety" or indeed what problem is being solved.
EDIT: Sorry, I just read the linked issue, I see the problem. Still, with thread isolation, it seems the change cause a thread to be launched and two connections checked out from the pool for every query or in_transaction?; these threads then proceed to be zombies because they're never joined. This doesn't seem right, especially if there's already a thread being launched by ActiveSupport. Even in the fiber case, some tests would be useful to illustrate.
|
@dividedmind thank you for your feedback. I forgot to flag this PR as a draft. Tests and a better description are coming 🙌🏻 And yes, the description was created by ChatGPT. I planned on updating it, along with adding tests and inline docs before it's ready for final review. I wanted to make this code available early for any feedback such as yours. 👍 |
…vels for :fiber and :thread
| ActiveRecord::Base.connection_pool.with_connection { yield } | ||
| end.resume | ||
| else | ||
| ActiveRecord::Base.connection_pool.with_connection { |conn| block.call(conn) } |
There was a problem hiding this comment.
I see no block in this context.
Attempt to fix #349
This PR fixes this issue by wrapping the Fiber that is accessing the connection pool with a temporary "root" Fiber. Once the query has completed, the fiber will be terminated and the connection will be returned to the pool.This PR is at a state where a fix can be applied and tested, but the implemented fix is not correct.