Dont clean connections on Integration tests#24500
Conversation
First, the action of `clear_active_connections!` should not be in the QueryCache executor. They have different responsabilites. This brings `ConnectionManagement` class back, now as an executor instead of Middleware, however it is only resposible for clearing the connections on complete. Also, this adds a flag, so we can turn off the clear action on Integration tests, otherwise we can return connections with open transactions back to the pool. [fixes #24491]
9a8a76d to
44115fb
Compare
| app.executor.register_hook(conn_hook) | ||
| end | ||
|
|
||
| ActiveSupport.on_load(:action_dispatch_integration_test) do |
There was a problem hiding this comment.
I'm not sure it's we want to disable it globally when ActionDispatch::IntegrationTest loaded.
For example we want to phantomjs end to end tests (clean_connections desired) as well as integration test (no clean connections) in the same run.
I think we should do the same as action_mailer:
There was a problem hiding this comment.
Yeah, make sense. I did some inline changes.
|
We'll also have the same issue with jobs, in particular test adapter. One solution is not calling them in execution wrap - mfazekas@5e5f89f. |
When the query cache completes, if Active Record is still inside of a transaction, it is because the transaction is meant to be left open above this unit of work (such as transactional fixtures in tests). There were several tests around the behavior of "tests" that were invalid, as tests are not run through the executor. They have been changed to reflect the new behavior, which is closer to what actually occurs in Rails tests. Fixes rails#23989 Fixes rails#24491 Close rails#24608 Close rails#24500
|
I think #24608 is better |
|
thanks @sgrif . I will take a look at your PR |
…4610) When the query cache completes, if Active Record is still inside of a transaction, it is because the transaction is meant to be left open above this unit of work (such as transactional fixtures in tests). There were several tests around the behavior of "tests" that were invalid, as tests are not run through the executor. They have been changed to reflect the new behavior, which is closer to what actually occurs in Rails tests. Fixes #23989 Fixes #24491 Close #24500
Problem
Connections are going back to the pool with open transactions. This can cause deadlocks.
[related to c7b7c6a]
Solution
First, the action of
clear_active_connections!should not be in theQueryCache executor. They have different responsibilities.
This brings
ConnectionManagementclass back, now as an executorinstead of Middleware, however it is only responsible for clearing the
connections on complete. [related to #23807]
Also, this adds a flag, so we can turn off the clear_connection action on
Integration tests.
[fixes #24491]
review @matthewd @jeremy @rafaelfranca @mfazekas
To test the deadlocks, i used a variation of @mfazekas's script, see https://gist.github.com/arthurnn/fbb59a684a75a415a2a8032c0ee99f54
I had to change it, because with this implementation, instead of using
rack.testenv var, the clear connections action is attached to the IntegrationTest class.