Use safe handles for SQLite interop

[sharwell]

No description provided.

[CyrusNajmabadi]

worth doc'ing if this is corrct wrt exceptions?

[sharwell]

I updated this to return false when the result is not Result.OK

[CyrusNajmabadi]

1. so, do all subclasses return true? if so, is there value in teh bool?
2. should we be looking at the return values of the native method we're calling?

[sharwell]

so, do all subclasses return true?

Currently, yes. false can be returned to indicate an error which is reported by an MDA during debugging.

if so, is there value in teh bool?

I kept it as close to ReleaseHandle as possible (same return type with the same meaning).

should we be looking at the return values of the native method we're calling?

If one of the return values means error, we could return false for that case. I'm neutral on this; the return value isn't used for anything aside from the debugger.

[CyrusNajmabadi]

this personally confuses me. the impl of sqlite3_open_v2 specifically gives back a bogus handle in the case of failure. so in that case, disposing it just serves to do no work. To me, it seems preferable to just return a handle?. client then knows they have to check that, and deal with the null case (which certainly wouldn't need to be disposed). Or, it succeeds, and you have a real handle you cangrab and pass along.

[sharwell]

this personally confuses me. the impl of sqlite3_open_v2 specifically gives back a bogus handle in the case of failure.

This precisely follows the behavior that would occur if the P/Invoke signatures were originally written to operate on safe handles instead of IntPtr (which is the preferable way to write the signatures since .NET 2.0).

[CyrusNajmabadi]

that's fine. i would personally prefer docs here then. because when i do the computation/data-flow in my head it basically indicates a smell here: i.e. i'm making a call that is certain to do nothing. if that's appropriate, we should at least doc. That way when i'm reading this 3 years from now i realize this is intentional and isn't just a line of code i'll throw out when doing cleanup :)

[sharwell]

Added docs

[CyrusNajmabadi]

i don't see the docs. can you push?

[sharwell]

They are in de40b56

[CyrusNajmabadi]

I like this a lot more :) just a few minor thoughts/suggestions on potential cleanup. Thanks!

[CyrusNajmabadi]

So, from the recent issue, it seems like the finalizer actually serves a good purpose (although it's clearly very unpleasant to have to investigate). if we move to this mode, should we at least make the storage service have a similar finalizer to catch clients that are forgetting to dispose it?

[sharwell]

if we move to this mode, should we at least make the storage service have a similar finalizer to catch clients that are forgetting to dispose it?

I generally do not believe this matters. If someone feels otherwise, we can always catch this with performance traces and other tools.

Another way to view this is: it's easy for us to maintain "mostly" deterministic release of resources, even without the finalizer. The finalizer only ever caught super edge cases, and those edge cases were never significant enough that it would have been a problem to just allow the finalizer to deal with them silently. Trying to get fancy with stopping every last case is more problematic than any benefit it would provide.

[CyrusNajmabadi]

I generally do not believe this matters. If someone feels otherwise, we can always catch this with performance traces and other tools.

I guess i find that somewhat unsatisfying. We had bugs where the system was being misused. The system properly was telling us: there's a leak, this is bad.

and those edge cases were never significant enough that it would have been a problem to just allow the finalizer to deal with them silently.

To me that's just papering over the issue (i.e. the original bug still remains, we're just unaware of it). Very specifically, these finalizers are intentionally not trying to make up for misbehaving code, but to call attention to it so that they can be made correct. I personally view that as a highly desirable property of the system, especially one that wraps a data-storage component.

[sharwell]

@CyrusNajmabadi There are a few ways a user could validate this cleanup even with safe handles, e.g. so it could run during integration tests. The obvious one would be adding a finalizer to the SafeHandle-derived type(s), but that is limited to handled controlled by the application and likely would only run during debug builds. With .NET Core, another approach is listening for GC events so the finalized objects can be tracked over a longer period of time to understand trends. The following example was provided by @stephentoub :

using System;
using System.Diagnostics.Tracing;
using System.IO;
using System.Threading;
public sealed class Program
{
    static void Main()
    {
        string path = Path.GetTempFileName();
        using GCListener listener = new GCListener();
        while (true)
        {
            File.OpenRead(path);
            Thread.Sleep(1000);
            GC.Collect(2, GCCollectionMode.Forced);
        }
    }
}
internal sealed class GCListener : EventListener
{
    protected override void OnEventSourceCreated(EventSource eventSource)
    {
        if (eventSource.Name.Equals("Microsoft-Windows-DotNETRuntime"))
        {
            EnableEvents(eventSource, EventLevel.Verbose, (EventKeywords)1);
        }
    }
    protected override void OnEventWritten(EventWrittenEventArgs eventData)
    {
        if (eventData.EventName.Contains("Finalize"))
        {
            Console.Write($"{eventData.EventName}: ");
            for (int i = 0; i < eventData.Payload.Count; i++)
            {
                Console.Write($", Name=\"{eventData.PayloadNames[i]}\" Value=\"{eventData.Payload[i]?.ToString() ?? string.Empty}\", ");
            }
            Console.WriteLine();
        }
    }
}

Prior to .NET Core, EventListener didn't have access to these GC events, but they were ported to EventSource so they would work with EventPipe. See NativeRuntimeEventSource.cs.

[CyrusNajmabadi]

should the ?? IntPtr.Zero just happen in the base class instead of all the derived ones?

[sharwell]

The ptr property isn't defined in an interface, and it seemed weird(er) to pass down IntPtr?.