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

Add comprehensive locking around DB transactions #28726

Merged
merged 1 commit into from Apr 12, 2017

Conversation

@matthewd
Copy link
Member

matthewd commented Apr 10, 2017

Transactional-fixture using tests with racing threads and inter-thread synchronisation inside transaction blocks will now deadlock... but without this, they would just crash.

In 5.0, the threads didn't share a connection at all, so it would've worked... but with the main thread inside the fixture transaction, they wouldn't've been able to see each other.

So: as far as I can tell, the set of operations this "breaks" never had a compelling use case. Meanwhile, it provides an increased level of coherency to the operational feel of transactional fixtures.

If this does cause anyone problems, they're probably best off disabling transactional fixtures on the affected tests, and managing transactions themselves.

Fixes #28197

Transactional-fixture using tests with racing threads and inter-thread
synchronisation inside transaction blocks will now deadlock... but
without this, they would just crash.

In 5.0, the threads didn't share a connection at all, so it would've
worked... but with the main thread inside the fixture transaction, they
wouldn't've been able to see each other.

So: as far as I can tell, the set of operations this "breaks" never had
a compelling use case. Meanwhile, it provides an increased level of
coherency to the operational feel of transactional fixtures.

If this does cause anyone problems, they're probably best off disabling
transactional fixtures on the affected tests, and managing transactions
themselves.
@matthewd matthewd requested a review from eileencodes Apr 10, 2017
@matthewd matthewd added this to the 5.1.0 milestone Apr 10, 2017
@matthewd
Copy link
Member Author

matthewd commented Apr 10, 2017

@benoitongit @KeithP are either of you able to confirm this solves the problem in your respective apps? It seemed to make #28197 (comment) happier, at least.

@benoitongit
Copy link

benoitongit commented Apr 11, 2017

Thanks @matthewd! works like a charm for me :)

@KeithP
Copy link
Contributor

KeithP commented Apr 12, 2017

Looks okay so far when running tests locally.
But on the build server most tests fail due to a "PG UndefinedTable" error.
Local PostgreSQL version is 9.3; Build server PostgreSQL version is 9.2.

@KeithP
Copy link
Contributor

KeithP commented Apr 12, 2017

When combined with PR #28740 this solves the issue for us. Thanks @matthewd

@matthewd matthewd merged commit 92ba89d into rails:master Apr 12, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
matthewd added a commit that referenced this pull request Apr 12, 2017
Add comprehensive locking around DB transactions
@matthewd
Copy link
Member Author

matthewd commented Apr 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants
You can’t perform that action at this time.