views:

260

answers:

3

Help me understand. I've read that

"The time and order of execution of finalizers cannot be predicted or pre-determined"

Correct?

However looking at RavenDB source code TransactionStorage.cs I see this

~TransactionalStorage()
{
try
{
 Trace.WriteLine(
  "Disposing esent resources from finalizer! You should call TransactionalStorage.Dispose() instead!");
 Api.JetTerm2(instance, TermGrbit.Abrupt);
}
catch (Exception exception)
{
  try
  {
   Trace.WriteLine("Failed to dispose esent instance from finalizer because: " + exception);
   }
   catch
   {
   }
 }
}

The API class (which belongs to Managed Esent) which presumable takes handles on native resources presumably using a SafeHandle?

So if I understand correctly the native handles SafeHandle can be finalized before TransactionStorage which could have undesired effects, perhaps why Ayende has added an catch all clause around this?

Actually diving into Esent code, it does not use SafeHandles.

According to CLR via C# this is dangerous?

    internal static class SomeType {  
   [DllImport("Kernel32", CharSet=CharSet.Unicode, EntryPoint="CreateEvent")]  

 // This prototype is not robust  
   private static extern IntPtr CreateEventBad( 
      IntPtr pSecurityAttributes, Boolean manualReset, Boolean initialState, String name);  


 // This prototype is robust  
  [DllImport("Kernel32", CharSet=CharSet.Unicode, EntryPoint="CreateEvent")]  
  private static extern SafeWaitHandle CreateEventGood( 
     IntPtr pSecurityAttributes, Boolean manualReset, Boolean initialState, String name)

  public static void SomeMethod() {  
     IntPtr         handle = CreateEventBad(IntPtr.Zero, false, false, null);  
     SafeWaitHandle swh    = CreateEventGood(IntPtr.Zero, false, false, null);  
  }  
}

Managed Esent (NativeMEthods.cs) looks like this (using Ints vs IntPtrs?):

  [DllImport(EsentDll, CharSet = EsentCharSet, ExactSpelling = true)]
    public static extern int JetCreateDatabase(IntPtr sesid, string szFilename, string szConnect, out uint dbid, uint grbit);

Is Managed Esent handling finalization/dispoal the correct way, and second is RavenDB handling finalizer the corret way or compensating for Managed Esent?

+2  A: 

There's a number of things wrong here. My comments:

  1. Finalizers must assume that finalizers have already run for every other class. This would include, say, classes writing out Trace.WriteLine statements to a log file.
  2. A catch clause may or may not work to protect against already-finalized classes. Usually finalizers do not throw, even if they fail to release their unmanaged resources (since that would normally crash the entire program).
  3. Managed ESENT certainly should be using SafeHandles, for reasons described here (namely, prevention of leaks when asynchronous exceptions are raised and also protecting against handle recycling). I'm really surprised that Laurion's not using best practices here.
  4. ESENT session IDs are IntPtrs, but their DB IDs are uints. Both of them should be wrapped in a SafeHandle-derived class, though.

In short, it looks like both projects are not handling finalization or handles correctly. It is a hard problem, as my first CodeProject article points out.

Before managed ESENT was released, I had started working on my own wrappers for the ESENT API, intending to release as OSS. After a few discussions with Laurion, I decided not to do so since Microsoft was going to do it too. The code I have is not feature-complete, but it does properly use SafeHandles, if you're interested.

Also note that ESENT has no backwards compatibility when writing (it will force upgrades, so a db from XP opened on Win7 can never be read by XP again). It's a fine choice if all data is local to a machine, but it can't be used if you need to copy the db to other machines.

Stephen Cleary
+3  A: 

Using SafeHandles with ESENT resources is very complex and dangerous, so I decided not to do it. There are two major problems:

  1. Unlike Win32 handles, ESENT handles are inter-related so that closing one handle will implicitly close others.
  2. It is not safe to close an already closed ESENT handle.

For the first point, there are several cases to be considered:

  • JetRollback will close all tables opened inside of the transaction but JetCommit will not.
  • JetEndSession will close all tables and databases opened by the session.
  • JetTerm can close all sessions, tables and databases opened by the instance.

Now a JET_SESID or JET_TABLEID is actually a pointer to an internal structure (we tried indirecting through a handle table but found that was too slow especially when used by multiple threads). That means once a resource is closed the memory can be re-used. Releasing the resource again might release another thread's resource; just like double-freeing a pointer.

That makes this code surprisingly challenging when it comes to finalization:

void Foo(JET_SESID sesid, JET_DBID dbid)
{
    JET_TABLEID tableid;

    Api.JetBeginTransaction(sesid);
    Api.JetOpenTable(sesid, dbid, "table", null, 0, OpenTableGrbit.None, out tableid);
    // do something...
    if (somethingFailed)
    {
        Api.JetRollback(sesid, RollbackTransactionGrbit.None);
    }
    else
    {
        Api.JetCommitTransaction(sesid, CommitTransactionGrbit.None);
    }
}

If JET_TABLEID is wrapped in a SafeHandle we have to know that the JetRollback() call (which doesn't even take the tableid as a parameter) has closed the table, so the finalizer cannot close the table. One other other hand, if we take the commit path then the finalizer should close the table.

If JET_SESID is also a SafeHandle then we have to keep track of the order that the finalizers are executed in. If the JET_SESID has already been finalized then we cannot close the JET_TABLEID.

Tracking the relations between instances, session, tables and transactions and then doing the right thing in a finalizer would be incredibly hard and would be best done using a more sophisticated object model than ManagedEsent provides.

We can, however, use a SafeHandle with a JET_INSTANCE because there are no APIs which can implicitly close an instance. The Instance() wrapper does that. Why not have JET_INSTANCE be a SafeHandle? There are cases where applications want to exit without terminating ESENT at all -- termination can be slow and with durable transactions you don't actually lose any information if you simply exit the program -- database recovery will run automatically at JetInit.

As far as finalizer order, I believe that critical finalizers (e.g. SafeHandles) always run after all normal finalizers have run.

Laurion Burchall
+1  A: 

I just would like to clarify something about SafeHandles. SafeHandles are not only Finalizable objects, they have a stronger gurantee of finalization called Critical Finalization Critical finalizer are guranteed to run after all the normal finalizers have already run. For this reason, you are guranteed that SafeHandles are the last things that get finalized.

Critical finalization is one of the feature that makes SafeHandle Safe:)

This link contains more information. Hope this helps.

mfawzymkh