Reject connections with no transactions open in specs
What does this MR do and why?
Reject connections with no transactions open in specs
Due to a race condition, it is possible for another thread (e.g.
ActionCable) to create a new DB connection. This new DB connection will
not have access to the transaction opened from the before_all_adapter
.
To workaround this issue, we prevent DB connections that has no access
to the before_all_adapter
transaction from being checked in. So that
this bad DB connection can't be used by tests by accident.
The race condition goes something like this:
- Transactional test starts
-
before_all_adapter
begins a DB transaction for each connection pool. We need this forbefore_all
andlet_it_be
to work with transactional tests. - Code checks out DB connections as usual. These DB connections are
re-used across threads using
lock_thread
, and thread connection cache. - A test finishes. DB connection is checked back in. This means the thread connection cache is empty. The checked in DB connection is now available.
- At this time, for the next test, the test thread also requests a DB connection to be checked out. It reuses the available connection.
- At the same time, another thread (ActionCable) requests a DB
connection to be checked out. Because the thread connection cache is
empty, and all available connections are taken, Rails has to create a new DB connection.
This DB connection has no access to the
before_all_adapter
transaction. - Around this time, the test thread populates the thread connection cache with the checked out DB connection.
- Tests continue to run.
- Another test finishes. DB connection is checked back in again.
- At the same time, the ActionCable execution completes. It starts
checking back in DB connections, including the DB connection that
has no access to the
before_all_adapter
transaction - Another test starts requesting a DB connection. The thread connection cache is empty, so we look in available DB connections. The first available DB connection is the bad one.
- Tests now fail with symptoms like:
Couldn't find User with 'id'=337
Related issue: #416663 (closed)
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
- Reduce
statement_timeout
inconfig/database.yml
to50s
export GITLAB_TEST_EAGER_LOAD=1
- Run
bundle exec rspec -f d spec/features/projects/work_items/work_item_spec.rb --seed 49620
Without this patch, you may see sometimes failures like:
Couldn't find User with 'id'=337
With this patch, you will sometimes see, but the specs pass:
Removing #<ActiveRecord::ConnectionAdapters::PostgreSQLAdapter:0x0000000129a41288> from pool as has no open transactions
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.