-
Notifications
You must be signed in to change notification settings - Fork 203
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
Add a busy_handler_timeout setter #443
Merged
tenderlove
merged 12 commits into
sparklemotion:main
from
fractaledmind:lock-wait-timeout
Jan 3, 2024
Merged
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
b05600e
Add a lock_wait_timeout setter to set a busy_handler that releases th…
fractaledmind 0b2ff58
Rename to busy_handler_timeout
fractaledmind ec687cf
Update test_integration_pending.rb
fractaledmind 6ef4db2
Simplify the busy_handler_timeout
fractaledmind 17c9db8
Update database.rb
fractaledmind e84b777
Write better tests of the busy_handler_timeout
fractaledmind a8d1715
Make the tests more expressive to make failures (why) easier to debug…
fractaledmind 77906c4
Try to create a more resilient test for the busy_handler_timeout
fractaledmind 59ecd77
Add additional assertion to ensure that timings are within a range
fractaledmind 9444b9b
Make test directly compare time using busy_timeout vs busy_handler_ti…
fractaledmind 48daae1
Fix bug with busy_handler_timeout and write better tests
fractaledmind 0c93d30
Ensure that db connections are closed
fractaledmind File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this sleep necessary? I think the VM is interruptible on any method call so the calls to clock_gettime etc should do the trick (I think).
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.
@tenderlove it's not about being interruptible, it's about forcibly releasing the GVL. The assumption is that the client that is currently holding the SQLite3 write lock might be another thread in the same process, hence we should try to switch back to it rather than busy loop for 100ms until the thread scheduler quantum is reached.
And even if it's not in that process, yielding the GVL allow other unrelated threads to proceed.
NB: for the "in same process case" a shared Ruby mutex would be way more efficient, but we'd need some way to tell two clients are pointing that the same database, hence should share a single Mutex.
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 see. In that case wouldn't
Thread.pass
also do the trick?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.
It would yes, but then the risk is that there's no other ready thread, causing the process to essentially be busy looping, which would pin one CPU to 100% and may not be desirable. A very short sleep actually make sense IMO.
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.
Sounds good to me!