Fix connection memory leak

This is a response to a memory leak that was witnessed in our stress
testing.

Below is a list of reading that should illuminate this change:
http://create.tpsitulsa.com/wiki/V8/Persistent_Handles

https://developers.google.com/v8/embed#handles

Basically, we were using a persistent handle to hold a reference to
ourselves in the Connection class.  This prevented the Collection
class from being collected properly.  The solution is to use a weak
persistent handle.

The node::ObjectWrap class, which the Connection class inherits from,
is a WeakHandle and manages the lifetime properly, calling the
destructor (provided it is virtual) of our instance  when it is being
collected.  A destructor was added to the Connection class that closes
the ODBC connection if necessary.

Since there are now two paths into closing a connection, it is
possible for a race condition to arise.  A critical section has been
put around the code that disconnects from SQL Server to prevent this.

For now this critical section code is Windows specific.  It will be
rewritten as cross platform at a later time.

Also, we were potentially leaking a persistent handle to the
OdbcConnection class in the OpenOperation class, meaning that the
OpenConnection class would never be collected.  To fix this, we call
the Dispose member function of the persistent handle to notify V8 that
we no longer need the pointer.

Microsoft Internal Review Id: jaykint-559c37537fb54de28ea1481d9bdf55cb

Author: jkint

src/Connection.cpp

@@ -44,13 +44,19 @@ namespace mssql
         NODE_SET_PROTOTYPE_METHOD(constructor_template, "beginTransaction", Connection::BeginTransaction);
         NODE_SET_PROTOTYPE_METHOD(constructor_template, "commit", Connection::Commit);
         NODE_SET_PROTOTYPE_METHOD(constructor_template, "rollback", Connection::Rollback);
-        NODE_SET_PROTOTYPE_METHOD(constructor_template, "nextResult", Connection::ReadNextResult);
+        NODE_SET_PROTOTYPE_METHOD(constructor_template, "nextResult", Connection::ReadNextResult);
 
         target->Set(String::NewSymbol("Connection"), constructor_template->GetFunction());
 
         OdbcConnection::InitializeEnvironment();
     }
-    
+
+    Connection::~Connection( void )
+    {
+        // close the connection now since the object is being collected
+        innerConnection->Collect();
+    }
+
     Handle<Value> Connection::Close(const Arguments& args)
     {
         HandleScope scope;
@@ -95,16 +101,18 @@ namespace mssql
         return scope.Close<Value>(connection->innerConnection->Rollback( callback ));
     }
 
-    Handle<Value> Connection::New(const Arguments& args) {
+    Handle<Value> Connection::New(const Arguments& args) 
+    {
         HandleScope scope;
 
         if (!args.IsConstructCall()) {
             return Undefined();
         }
 
-        Connection *s = new Connection();
-        s->Wrap(args.This());
-        s->This = Persistent<Object>::New(args.This());
+        Connection *c = new Connection();
+
+        c->Wrap(args.This());
+
         return args.This();
     }
 

src/Connection.h

@@ -38,9 +38,8 @@ namespace mssql
             : innerConnection(new OdbcConnectionBridge())
         {
         }
-        ~Connection()
-        {
-        }
+
+        virtual ~Connection();
 
         static void Initialize(Handle<Object> target);
         static Handle<Value> Close(const Arguments& args);

src/CriticalSection.h

@@ -0,0 +1,71 @@
+//---------------------------------------------------------------------------------------------------------------------------------
+// File: CriticalSection.h
+// Contents: Wrapper for a critical section that handles scope
+// 
+// Copyright Microsoft Corporation and contributors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+//
+// You may obtain a copy of the License at:
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+//---------------------------------------------------------------------------------------------------------------------------------
+
+namespace mssql {
+
+   class CriticalSection {
+
+   public:
+
+       CriticalSection()
+       {
+           InitializeCriticalSection( &handle_ );
+       }
+
+       ~CriticalSection()
+       {
+           DeleteCriticalSection( &handle_ );
+       }
+
+       void lock( void )
+       {
+           EnterCriticalSection( &handle_ );
+       }
+
+       void unlock( void )
+       {
+           LeaveCriticalSection( &handle_ );
+       }
+
+   private:
+
+       CRITICAL_SECTION handle_;
+   };
+
+   class ScopedCriticalSectionLock {
+
+   public:
+
+       ScopedCriticalSectionLock( CriticalSection& cs ) :
+           criticalSection_( cs )
+       {
+           criticalSection_.lock();
+       }
+
+       ~ScopedCriticalSectionLock( void )
+       {
+           criticalSection_.unlock();
+       }
+
+   private:
+
+       CriticalSection& criticalSection_;
+   };
+
+}  // mssql

src/OdbcConnection.cpp

@@ -41,21 +41,26 @@ namespace mssql
 
     bool OdbcConnection::TryClose()
     {
-        if (connectionState != Closed)
+        if (connectionState != Closed)  // fast fail before critical section
         {
-            SQLRETURN ret = SQLDisconnect(connection);
-            if (ret == SQL_STILL_EXECUTING) 
-            { 
-                return false; 
-            }
-            if (!SQL_SUCCEEDED(ret)) 
-            { 
-                connection.Throw();  
-            }
+            ScopedCriticalSectionLock critSecLock( closeCriticalSection );
+            if (connectionState != Closed)
+            {
+                SQLRETURN ret = SQLDisconnect(connection);
+                if (ret == SQL_STILL_EXECUTING) 
+                { 
+                    return false; 
+                }
+                if (!SQL_SUCCEEDED(ret)) 
+                { 
+                    connection.Throw();  
+                }
 
-            statement.Free();
-            connection.Free();
-            connectionState = Closed;
+                resultset.reset();
+                statement.Free();
+                connection.Free();
+                connectionState = Closed;
+            }
         }
 
         return true;

src/OdbcConnection.h

@@ -20,6 +20,7 @@
 #pragma once
 
 #include "ResultSet.h"
+#include "CriticalSection.h"
 
 namespace mssql
 {
@@ -31,6 +32,7 @@ namespace mssql
         static OdbcEnvironmentHandle environment;
         OdbcConnectionHandle connection;
         OdbcStatementHandle statement;
+        CriticalSection closeCriticalSection;
 
         enum ConnectionStates
         {

src/OdbcConnectionBridge.h

@@ -47,6 +47,12 @@ namespace mssql
             return scope.Close(Undefined());
         }
 
+        void Collect( void )
+        {
+            Operation* operation = new CollectOperation(connection);
+            Operation::Add( operation );
+        }
+
         Handle<Value> BeginTransaction(Handle<Object> callback )
         {
             HandleScope scope;

src/OdbcOperation.cpp

@@ -64,8 +64,6 @@ namespace mssql
 
             callback->Call(Undefined().As<Object>(), argc, args);
         }
-
-        callback.Dispose();
     }
 
 }

src/OdbcOperation.h

@@ -45,6 +45,11 @@ namespace mssql
         {
         }
 
+        virtual ~OdbcOperation()
+        {
+            callback.Dispose();
+        }
+ 
         virtual bool TryInvokeOdbc() = 0;
         virtual Handle<Value> CreateCompletionArg() = 0;
 
@@ -58,13 +63,19 @@ namespace mssql
         wstring connectionString;
         Persistent<Object> backpointer;
     public:
-        OpenOperation(shared_ptr<OdbcConnection> connection, const wstring& connectionString, Handle<Object> callback, Handle<Object> backpointer)
+        OpenOperation(shared_ptr<OdbcConnection> connection, const wstring& connectionString, Handle<Object> callback, 
+                      Handle<Object> backpointer)
             : OdbcOperation(connection, callback), 
               connectionString(connectionString), 
               backpointer(Persistent<Object>::New(backpointer))
         {
         }
 
+        virtual ~OpenOperation( void )
+        {
+            backpointer.Dispose();
+        }
+
         bool TryInvokeOdbc() override
         {
             return connection->TryOpen(connectionString);
@@ -195,6 +206,32 @@ namespace mssql
 
     };
 
+    class CollectOperation : public OdbcOperation
+    {
+    public:
+        CollectOperation(shared_ptr<OdbcConnection> connection)
+            : OdbcOperation(connection, Handle<Object>())
+        {
+        }
+
+        bool TryInvokeOdbc() override
+        {
+            return connection->TryClose();
+        }
+
+        Handle<Value> CreateCompletionArg() override
+        {
+            assert( false );
+            HandleScope scope;
+            return scope.Close( Undefined() );
+        }
+
+        // override to not call a callback
+        void CompleteForeground() override
+        {
+        }
+    };
+
     class BeginTranOperation : public OdbcOperation
     {
     public:

src/stdafx.h

@@ -26,6 +26,8 @@
 
 #include <sqlucode.h>
 
+#include <windows.h>	// for critical section until xplatform
+
 #include <vector>
 #include <queue>
 #include <string>