Disable transactions in locking examples #18089
Merged
Conversation
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
jrafanie
commented
Oct 11, 2018
| @@ -1,16 +1,15 @@ | |||
| require "concurrent/atomic/event" | |||
|
|
|||
| describe VmdbDatabaseConnection do | |||
| before do | |||
| @db = FactoryGirl.create(:vmdb_database) | |||
There was a problem hiding this comment.
yay, this isn't needed and is actually a problem now that I disabled transactions for these examples.
|
Enjoy my 📖 @NickLaMuro ... although I lifted it from rails' release notes.. |
NickLaMuro
reviewed
Oct 11, 2018
These tests are creating threads and explicit transactions to verify when we're locked or not locked in the database. The Rails 5.1 release notes mention that this type of behavior can cause a deadlock, which we've seen with rails 5.1, and to instead disable transactional tests. From the rails 5.1 release notes: Transactional tests now wrap all Active Record connections in database transactions. When a test spawns additional threads, and those threads obtain database connections, those connections are now handled specially: The threads will share a single connection, which is inside the managed transaction. This ensures all threads see the database in the same state, ignoring the outermost transaction. Previously, such additional connections were unable to see the fixture rows, for example. When a thread enters a nested transaction, it will temporarily obtain exclusive use of the connection, to maintain isolation. If your tests currently rely on obtaining a separate, outside-of-transaction, connection in a spawned thread, you'll need to switch to more explicit connection management. If your tests spawn threads and those threads interact while also using explicit database transactions, this change may introduce a deadlock. The easy way to opt out of this new behavior is to disable transactional tests on any test cases it affects. https://guides.rubyonrails.org/5_1_release_notes.html
jrafanie
force-pushed
the
disable_transactional_tests_for_locking_tests_to_avoid_deadlock_on_5_1
branch
from
32c7c4a
to
ff6808b
Compare
|
Checked commit jrafanie@ff6808b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
NickLaMuro
approved these changes
Oct 15, 2018
thanks to @NickLaMuro's research... 🙇 |
jrafanie
deleted the
disable_transactional_tests_for_locking_tests_to_avoid_deadlock_on_5_1
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
These tests are creating threads and explicit transactions to verify
when we're locked or not locked in the database. The Rails 5.1 release
notes mention that this type of behavior can cause a deadlock, which
we've seen with rails 5.1, and to instead disable transactional tests.
From the rails 5.1 release notes:
"Transactional tests now wrap all Active Record connections in database
transactions.
When a test spawns additional threads, and those threads obtain database
connections, those connections are now handled specially:
The threads will share a single connection, which is inside the managed
transaction. This ensures all threads see the database in the same
state, ignoring the outermost transaction. Previously, such additional
connections were unable to see the fixture rows, for example.
When a thread enters a nested transaction, it will temporarily obtain
exclusive use of the connection, to maintain isolation.
If your tests currently rely on obtaining a separate,
outside-of-transaction, connection in a spawned thread, you'll need to
switch to more explicit connection management.
If your tests spawn threads and those threads interact while also using
explicit database transactions, this change may introduce a deadlock.
The easy way to opt out of this new behavior is to disable transactional
tests on any test cases it affects."
https://guides.rubyonrails.org/5_1_release_notes.html