fix: closing db on finalizer and memory leak #177
Conversation
As part of the cache work, a finalizer was put on the sqlair.DB that would close the underlying sql.DB when the sqlair.DB went out of memory. This was a misfeature, if thread seperatly held the sql.DB or the same thread tried to use it later on it would be inexplicably closed. Remove the db.Close on the sqlair.DB finalizer.
When statements are eviceted from the cache, they are not always closed. This change fixes that. Some sqlair.Statements can have multiple sql queries generated from them depending on the arguements (e.g. bulk inserts, or queries with slice arguements). SQLair maintains a sqlair.Statement to driverStmt cache for DB queries. When one of these queries is executed, and it is looked up in the cache, is a driver statement is found, but has different SQL, it will be removed from the cache and replaced with the newly generated SQL from the version of the query with different arguments. Before this change, a finalizer was set on the *sql.Stmt to close it when it dropped from memory. The *sql.Stmt couldn't be closed straight away becuase there is a possibility someone else is using it concurrently, therefor the finalizer was set to clear it up after it had been finished with. This didn't work as intended. In fact, the sql.DB holds a reference to this *sql.Stmt so the finalizer will only ever be run when this is closed. The test designed to check that this statement was closed did not pick it up becuase before the check, the DB was closed. In a recent change, the DB stopped being closed at this point, and this caused the test to start failing. After this change, instead of putting a finalizer on the database/sql controller *sql.Stmt, we put a finalizer on the SQLair controller driverStmt. This means we can audit usages far more effectivly and are not dependent on third part code. If the finalizer is run before all concurrent users of the *sql.Stmt have finished with it, sql.Rows may return an error saying query closed. This happens when the underlying statement is closed. To ensure that the finalizer is only run when all concuirrent users have finished with it, the Iterator will now hold a reference to this driverStmt. That way, it will only be once the Iterator goes out of memory that the driverStmt is closed.
cache.go
Outdated
| // close it once concurrent users have finished with it and replace it with | ||
| // ours. | ||
| if ds, ok := sc.stmtDBCache[s.cacheID][db.cacheID]; ok { | ||
| runtime.SetFinalizer(ds, func(ds *driverStmt) { |
There was a problem hiding this comment.
Isn't this the same as setting it on the way in? Why not do that so that you don't have to do the lookup each time you set it?
There was a problem hiding this comment.
It would have the same effect, but we are only setting this finalizer in the very specific case that a sqlair.Statement is executed with arguments that cause different SQL to be generate to previous executions, as can happen with a bulk insert or slice query.
This way, we are only setting the finalizer if this conflict has occurred, i.e. we don't set this finalizer on most driverStmts. Setting it on the way in would mean we set it for all driverStmts we create.
If we made this change we would avoid the lookup, but we would still have to set a finalizer here on the newly created driverStmt
We already set a finalizer on the sqlair.Statement when it is created but this could not be replaced with finalizers on the driverStmts because the driverStmts would then never be removed from the cache.
There was a problem hiding this comment.
It would be safer if this was not a closure.
There was a problem hiding this comment.
Technically you set if for all statements -1 i.e. except for the last one to (ever) enter the cache.
It is not unsafe to set it unconditionally (if no one references it, they can't use it), but you drop the lookup. The added simplicity is worth the change.
There was a problem hiding this comment.
I don't see how we set this for all statements -1. This is only set when you have a sqlair.Statement that has been executed with new arguments that cause the SQL generated in BindInputs to come out different to the SQL that was generated for the previous execution. e.g. if you have a bulk insert inserting 3 items instead of 2.
In that case, when the bulk insert is run with 3 items, the lookup in Query will fail, which will cause the stmtCache.driverPrepareStmt to be run. When it hits the if statement that sets the finalizer, it will find the bulk insert sql with 2 arguments, set a finalizer on it, and implicitly evict it by overwriting it below.
In the case of a sqlair.Statement that never changes the SQL it generates driverPrepareStmt will only be run once (as the lookup will always subsequently succeed), and when it is the if statement that conditionally sets the finalizer will fail as the cacheID of the sqlair.Statement is not yet in the cache.
There was a problem hiding this comment.
It would be safer if this was not a closure.
Changed to be a function with a comment, also updated the comment above the setting of this finalizer to make things clearer.
There was a problem hiding this comment.
Maybe this would be better if it used the new AddCleanup method.
SQLair is on
Cool, good to know they're bringing this in, though SQLair is pinned to go 1.18 so we can't to take advantage of it. |
Go only supports 2 releases at a time. If you've got a project with 1.18, then we would need to understand why. Having old releases is a potential security concern. Juju is pushing forward with 1.23 and will move to 1.24 when possible. Even if sqlair doesn't go to 1.24, it probably should start looking at closing the window on 1.18 and moving to a more sensible version. You can tag a version with 1.18 support, that will allow projects to compile with that, if you want the more recent changes, either backporting to that tag, or moving to the latest should be the practice. |
That's fair, the decision was made to maximize compatibility when the project was started, though right now I am only aware of SQLair being actively used in Juju and Notary. Notary uses go 1.22. People are using the project with the caveat that it is still in active development and things may change, so in my opinion it would be justifiable to upgrade to version 1.22. It might be useful to tag a version 0.1 and branch it off on 1.18 though in case there was ever a need to release a security fix on that branch. For a version 1.0 though I don't think there would be any need to keep it on 1.18. |
fix: remove close DB on sqlair.DB going out of memory
As part of the cache work, a finalizer was put on the sqlair.DB that
would close the underlying sql.DB when the sqlair.DB went out of memory.
This was a misfeature, if thread seperatly held the sql.DB or the same
thread tried to use it later on it would be inexplicably closed.
Remove the db.Close on the sqlair.DB finalizer.
fix: memory leak
When statements are eviceted from the cache, they are not always closed.
This change fixes that.
Some sqlair.Statements can have multiple sql queries generated from them
depending on the arguements (e.g. bulk inserts, or queries with slice
arguements). SQLair maintains a sqlair.Statement to driverStmt cache for
DB queries. When one of these queries is executed, and it is looked up
in the cache, if a driver statement is found, but has different SQL, it
will be removed from the cache and replaced with the newly generated SQL
from the version of the query with different arguments.
Before this change, a finalizer was set on the *sql.Stmt to close it
when it dropped from memory. The *sql.Stmt couldn't be closed straight
away because there is a possibility someone else is using it
concurrently, therefor the finalizer was set to clear it up after it had
been finished with.
This didn't work as intended. In fact, the sql.DB holds a reference to
this *sql.Stmt so the finalizer will only ever be run when this is
closed. The test designed to check that this statement was closed did
not pick it up because before the check, the DB was closed. In a recent
change, the DB stopped being closed at this point, and this caused the
test to start failing.
After this change, instead of putting a finalizer on the database/sql
controlled *sql.Stmt, we put a finalizer on the SQLair controlled
driverStmt. This means we can audit usages far more effectively and are
not dependent on third party code.
If the finalizer is run before all concurrent users of the *sql.Stmt
have finished with it, sql.Rows may return an error saying query closed.
This happens when the underlying statement is closed. To ensure that the
finalizer is only run when all concurrent users have finished with it,
the Iterator will now hold a reference to this driverStmt. That way,
it will only be once the Iterator goes out of memory that the driverStmt
is closed.