search

ericsink / SQLitePCL.raw

A Portable Class Library (PCL) for low-level (raw) access to SQLite
Apache License 2.0
512 stars 106 forks source link

SafeHandle implementations do not lease owning objects #398

Open sharwell opened 3 years ago

sharwell commented 3 years ago

The sqlite3_blob implementation does not ensure it is released prior to the owning sqlite3 object. One of the following is needed:

  1. Document that sqlite3 is allowed to be disposed before sqlite3_blob (and more specifically, internal_sqlite3_close_v2 is allowed to be caled before internal_sqlite3_blob_close).
  2. Update the implementation of sqlite3_blob to lease the sqlite3 handle, which guarantees the finalization order. An example of this behavior using the v1 API is available in SafeSqliteBlobHandle.

Without the above, the new SafeHandle implementations are not directly usable, and must be wrapped in a second layer of safe handles to provide the ordering guarantees.

This issue applies to sqlite3_stmt (which dotnet/roslyn uses), and may apply to other types as well (not used by dotnet/roslyn). Please feel free to let me know if you have any questions and/or to mention me on a review.

ericsink commented 3 years ago

That's interesting. Thanks for the info.

Just curious -- did you find this issue because some problem showed up in the wild? Or did you notice it on code review?

sharwell commented 3 years ago

We found this issue during our work to update dotnet/roslyn from v1 to v2.

ericsink commented 3 years ago

Thinking more about this.

Issues like this one make me wonder if it was a mistake to migrate these types to use SafeHandle.

The goal of the lowest layer of SQLitePCLRaw is, as much as possible, to present the exact same semantics as the SQLite C API.

For example, in the SQLite C API, it is illegal to close a sqlite3 if it still has sqlite3_stmt instances open. Nothing ever calls sqlite3_close() automatically, and there is certainly no way to have sqlite3_close() find all sqlite3_stmts that are open and close them down first.

Right now I'm wondering if it would have been better if I had one layer that exactly matched the SQLite C API and everything related to SafeHandle and finalizers was one layer about that.

ericsink commented 3 years ago

Still thinking...

Document that sqlite3 is allowed to be disposed before sqlite3_blob

I wonder how credible it would be for me to argue that this documentation is already present and exists here:

https://www.sqlite.org/c3ref/close.html

sharwell commented 3 years ago

@ericsink I believe that is a reasonable claim to make. The only remaining question is whether the sqlite3_blob is usable while the connection is in the zombie state.

sebastienros commented 3 years ago

Please let me know if you think the issue I am describing is not related, I will file a new one, but it's also related to SafeHandle.

I was looking at a PerfView trace for an application using sqlite and the GC stats show a lot of finalized objects:

SQLitePCL.hook_handle 12434

This happens if the object's finalizer is called, and it's not expected if it implements IDisposable since it should call GC.SuppressFinalize() in that case. It happens that hook_handle inherits from SafeHandle which correctly invokes GC.SuppressFinalize(), so it leads me to think that some instances of hook_handle are not correctly disposed.

sharwell commented 3 years ago

@sebastienros That's a separate issue. hook_handle.ForDispose() prevents instances created to represent null from getting disposed. The solution is to delete hook_handle.ForDispose() since it's unnecessary (disposing of a null/invalid SafeHandle is not only fine; it's expected).