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

Remove Advisory lock #25

Merged
merged 23 commits into from
Nov 6, 2024
Merged

Remove Advisory lock #25

merged 23 commits into from
Nov 6, 2024

Conversation

brendon
Copy link
Owner

@brendon brendon commented Oct 13, 2024

This is in favour of scope locks or locking all the records in a table if no scope exists.

@brendon
Copy link
Owner Author

brendon commented Oct 17, 2024

@fractaledmind, sorry this is a little out of the blue, but I wondered if you had any thoughts about why my sqlite tests might be deadlocking? I've had a go at integrating your gem but there still appears to be a problem. The problem test in particular is:

def test_no_duplicate_row_values_when_creating
ActiveRecord::Base.connection_handler.clear_all_connections!
list = List.create name: "List"
students = []
10.times do
threads = 20.times.map do
Thread.new do
ActiveRecord::Base.connection_pool.with_connection do
students << list.authors.create(name: "Student", type: "Author::Student")
end
end
end
threads.each(&:join)
end
assert_equal (1..students.length).to_a, list.authors.map(&:position)
list.destroy
end

I'd love to keep sqlite as a compatible database for this gem but am getting close to dropping it as the ability to keep the list ordering clean is the primary purpose of this library :)

I frequently encountered your work on getting sqlite working well in production for Rails. Well done! :)

@fractaledmind
Copy link

Let me dig in tonight and get back to you.

@brendon
Copy link
Owner Author

brendon commented Oct 18, 2024

Thanks @fractaledmind :D

@rossme
Copy link

rossme commented Oct 27, 2024

Hi @brendon, I was running the tests earlier and I had to increase the pool size to 20 in database.yml which solved the issue for me. Not sure if that helps or not. I've used acts_as_list and this is a terrific improvement over that gem. Nice job.

@brendon
Copy link
Owner Author

brendon commented Oct 28, 2024

Thanks @rossme, that doesn't seem to have helped in the CI environment. I feel like there's something different about it vs the Mac environment.

@brendon
Copy link
Owner Author

brendon commented Oct 28, 2024

Thank you for your kind words too! :D

@brendon
Copy link
Owner Author

brendon commented Oct 29, 2024

Managed to remove default_transaction_mode: IMMEDIATE also. Maybe the tests aren't robust enough though.

@brendon
Copy link
Owner Author

brendon commented Oct 29, 2024

Also managed to remove the activerecord-enhancedsqlite3-adapter gem. Sqlite tests pass with or without the row locking (as expected). They fail for the other databases without the locking, I guess indicating that the sqlite connection pool only gives write access one at a time to each thread which is what the row locking does for other databases. Just conjecture on my part.

Has anyone tried these modifications in production?

@brendon brendon merged commit 0d7c788 into main Nov 6, 2024
36 checks passed
@brendon brendon deleted the advisory-lock branch November 6, 2024 21:42
@brendon
Copy link
Owner Author

brendon commented Nov 6, 2024

I've merged this into main and will do a new minor release since there is a breaking change. Please get in touch if you notice any issues.

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.

3 participants