Do not attempt to return connection with open transaction to pool#24610
Conversation
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
|
r? @matthewd |
|
My concern with this is that it means we'll leak connections if (for whatever reason) there's a transaction left open. Obviously returning them to the pool is also unwise, but IMO we should more likely be raising, or discarding the connection, if we "finish" with it dirty. To me #24608 is more correct, both because the test is a unit of work, and most critically because the test body is user code that can trigger a load -- without it, we lose thread safety. |
|
The only way that a transaction would be opened (in a way that Additionally, unless I'm missing something, this code would be extremely broken if the transactions were leaking, as PG is the only backend which overrides |
|
Perhaps another approach might be to check if the connection is established when the unit of work begins, and only return it if the connection was established as part of this work. But more generally my concern is that this code is making some major assumptions about the ownership of the connection, with no verification that they are true. And |
| ActiveRecord::Base.clear_active_connections! | ||
| unless ActiveRecord::Base.connection.transaction_open? | ||
| ActiveRecord::Base.clear_active_connections! | ||
| end |
There was a problem hiding this comment.
I think it should be more like this, as ActiveRecord::Base.clear_active_connections! will clear current thread connection on all connection pool
ActiveRecord::Base.connection_handler.connection_pool_list.each do |pool|
pool.release_connection unless pool.active_connection? && pool.connection.transaction_open?
endThere was a problem hiding this comment.
Given that the discussion here is about the ownership of the connection for the current thread, and whether we should try to release it, the pool shouldn't be involved at all here. Clearing unused connections from other threads is a side effect of checking the current connection in, not the main goal.
There was a problem hiding this comment.
@sgrif my code should not be deal with other thread's connections. It tries to deal with current thread's connections on multiple connection pools.
In other words it's about multiple connections (to different databases) of the current thread - maybe only some of them has a transaction opened.
|
I think #24608 is neccesary, and yes ideally this one would verify the assumptions about connections at the run callback, but it might be better to raise if the assumption is not correct. That is likely a result of a configuration error - user should add outer executor vs a runtime error. |
|
So after thinking about it a bit more, I'm more convinced that this is the right way forward. Ideally our semantics would instead be something like: "check if a connection is already established when the unit of work begins. If not, establish one, and return it to the pool at the end of the unit of work". However, I think that would be much more likely to leak connections that this path is, simply because a middleware somewhere might establish a connection before we get here without realizing the implications. Unfortunately, I think this is just a side effect of the fact that the query cache held the responsibility for managing the connection pool, rather than something more explicitly meant to keep that role. Given that we haven't had clear semantics on the ownership roles at play here, any significant changes would likely cause subtle breakage. As such, I think dealing with a transaction being open as our main indicator is both closest to the previous condition of |
|
@matthewd WDYT re ---^? |
|
I did come close to implementing it that way ("leave the connection in whatever state it was when we started")... the reasons I didn't were twofold: 1) the danger that something else makes the connection before we run, as you've described, and 2) I realised the test already needed to be wrapped by the executor for thread safety, which obviated this behaviour. So, I don't feel strongly that we shouldn't make the change you're describing... but I do believe it only solves one of two problems. |
|
Going to go forward with this, as I think we've addressed the only concern with this specific change. #24608 is likely a change we should make in addition to this. |
|
@sgrif i don't think my comment regarding multiple database connections was addressed, was it? #24610 (comment) As a concrete case: If you have the default connection to sqlite and another to postgresql, but you only have fixtures for classes stored in postgresql, then no transaction will be opened on sqlite - |
|
If it can be demonstrated through the public API only, then yes. That doesn't sound like something that can occur in a normal Rails app, though. |
|
@sgrif so this one reproduces deadlock using public API but with multiple db connections: I agree it's not a normal rails app, but i don't think it deserves this kind of punishment. I was wrong about rails fixtures, as |
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