search

crawshaw / sqlite

Go SQLite3 driver
ISC License
561 stars 67 forks source link

sqlite.Stmt.Finalize should panic if called on a Stmt returned from Conn.Prepare #129

Closed AdamSLevy closed 2 years ago

AdamSLevy commented 2 years ago

sqlite.Stmt.Finalize should be called by users only on Stmts returned from Conn.PrepareTransient, but never on Stmts returned from Conn.Prepare.

This is a common coding error that we could help people avoid by panicking in Stmt.Finalize if called on Stmts returned from Conn.Prepare.

AdamSLevy commented 2 years ago

I have a second thought about this. Why do we expose Finalize anyway? Really there is no need to call Finalize unless the statement is being garbage collected. Should we just add a runtime.SetFinalizer call and have the garbage collector call sqlite3_finalize whenever a Stmt becomes unreachable? The only disadvantage I see is that explicitly calling Stmt.Finalize may improve memory performance because statements won't be kept around until the next GC run. But this would greatly simplify the API and a common cause of coding errors. @anacrolix any thoughts on that approach?

https://sqlite.org/c3ref/finalize.html https://pkg.go.dev/runtime#SetFinalizer

anacrolix commented 2 years ago

Interestingly if you don't finalize statements before the connection is closed, the connection is put into a zombie mode. While Go is a garbage collected language, I don't think the interface exposed here this far relies on GC, and I don't think it should in just one area.

It's fine to finalize cached statements. I think it might be more consistent to finalize transient statements automatically on connection close too. However the zombie mode stuff might actually prefer that all statements are manually finalized when they're no longer in use, not just the transient ones.

I think there's more to consider here for consistency.

To address the original issue though, cached statements should not panic when finalized, and I think it's perfectly legitimate and correct to be finalizing them prematurely.

AdamSLevy commented 2 years ago

@anacrolix Thanks.

Why is it okay to finalize cached statements? That seems incorrect since a statement cannot be used after it has been finalized. As such if you finalize a cached statement, then the next time you call Conn.Prepare you will be returned the finalized and useless statement. That definitely seems like incorrect design.

anacrolix commented 2 years ago

That's not my reading: https://github.com/crawshaw/sqlite/blob/master/sqlite.go#L558.

To emphasise the preference for not relying on the garbage-collector in Go for resource management: The defer/Close pattern generally means you can expect to deterministically release resources (manually) in Go programs. I'm not sure what the behaviour would be if you had Stmts that needed GC finalizers to clean up conns (whether in zombie mode or not), and what that means when the process finally exits if they have run yet. That's not to say GC doesn't have its use in Go, but the pattern in this project seems to be to not make use of it. If you did make use of GC, you would likely want to do it consistently throughout the project. My projects at least wouldn't gain much from that. There could be some value in changing the GC finalizer for transient Stmts to actually finalize the sqlite C stmt?

AdamSLevy commented 2 years ago

You're right. Thank you. Having been away from this code base for some time, clearly my mental model is off in some places. Glad you're here to sanity check things. Really appreciate it. I'm going to go ahead and close this as its based on a faulty premise.