better handling of network related timeouts

With a working network connection:
* command batch is called on non gvl thread
* tinytds_err_handler is called with timeout error and returns INT_TIMEOUT
* dbcancel is called and command batch returns
* nogvl_cleanup is called and timeout error is raised

This is all great. The timeout is hit, the db connection lives, and a error is thrown.

With a network failure:
* command batch is called on non gvl thread
* tinytds_err_handler is called with timeout error and returns INT_TIMEOUT
* dbcancel is called and does not succeed. command batch never returns
* nogvl_cleanup is not called

This is not great. dbcancel does not succeed because of the network failure. the command batch does not return until the underlying network timeout on the os is hit. TinyTds doesn't throw an error in the expected timeout window.

To fix, we set a flag when a timeout is encountered. We use dbsetinterrupt to check this flag periodically while waiting on a read from the server. Once the flag is set the interrupt with send INT_CANCEL causing the pending command batch to return early. This means nogvl_cleanup will be called and raise the timeout error.

This shouldn't have any affect in "normal" timeout conditions due to the fact that dbcancel will actually succeed and cause the normal flow before the interrupt can be called/handled. This is good because in these situtations we still want the dbproc to remain in working condition.

Author: bvogelzang

ext/tiny_tds/client.c

@@ -96,7 +96,13 @@ int tinytds_err_handler(DBPROCESS *dbproc, int severity, int dberr, int oserr, c
       but we don't ever want to automatically retry. Instead have the app
       decide what to do.
       */
-      return_value = INT_TIMEOUT;
+      if (userdata->timing_out) {
+        return INT_CANCEL;
+      }
+      else {
+        userdata->timing_out = 1;
+        return_value = INT_TIMEOUT;
+      }
       cancel = 1;
       break;
 
@@ -172,6 +178,33 @@ int tinytds_msg_handler(DBPROCESS *dbproc, DBINT msgno, int msgstate, int severi
   return 0;
 }
 
+/*
+Used by dbsetinterrupt -
+This gets called periodically while waiting on a read from the server
+Right now, we only care about cases where a read from the server is
+taking longer than the specified timeout and dbcancel is not working.
+In these cases we decide that we actually want to handle the interrupt
+*/
+static int check_interrupt(void *ptr) {
+  GET_CLIENT_USERDATA((DBPROCESS *)ptr);
+  return userdata->timing_out;
+}
+
+/*
+Used by dbsetinterrupt -
+This gets called if check_interrupt returns TRUE.
+Right now, this is only used in cases where a read from the server is
+taking longer than the specified timeout and dbcancel is not working.
+Return INT_CANCEL to abort the current command batch.
+*/
+static int handle_interrupt(void *ptr) {
+  GET_CLIENT_USERDATA((DBPROCESS *)ptr);
+  if (userdata->timing_out) {
+    return INT_CANCEL;
+  }
+  return INT_CONTINUE;
+}
+
 static void rb_tinytds_client_reset_userdata(tinytds_client_userdata *userdata) {
   userdata->timing_out = 0;
   userdata->dbsql_sent = 0;
@@ -391,6 +424,7 @@ static VALUE rb_tinytds_connect(VALUE self, VALUE opts) {
       }
     }
     dbsetuserdata(cwrap->client, (BYTE*)cwrap->userdata);
+    dbsetinterrupt(cwrap->client, check_interrupt, handle_interrupt);
     cwrap->userdata->closed = 0;
     if (!NIL_P(database) && (azure != Qtrue)) {
       dbuse(cwrap->client, StringValueCStr(database));

ext/tiny_tds/result.c

@@ -94,6 +94,7 @@ static void nogvl_setup(DBPROCESS *client) {
 static void nogvl_cleanup(DBPROCESS *client) {
   GET_CLIENT_USERDATA(client);
   userdata->nonblocking = 0;
+  userdata->timing_out = 0;
   /*
   Now that the blocking operation is done, we can finally throw any
   exceptions based on errors from SQL Server.

test/client_test.rb

@@ -129,27 +129,24 @@ class ClientTest < TinyTds::TestCase
       end
     end
 
-    it 'must run this test to prove we account for dropped connections' do
-      skip
+    it 'raises TinyTds exception with sql batch timeout due to network failure' do
+      skip if ENV['CI'] && ENV['APPVEYOR_BUILD_FOLDER'] # only CI using docker
       begin
-        client = new_connection :login_timeout => 2, :timeout => 2
+        client = new_connection timeout: 2
         assert_client_works(client)
-        STDOUT.puts "Disconnect network!"
-        sleep 10
-        STDOUT.puts "This should not get stuck past 6 seconds!"
+        docker_container('pause', wait_for: 1)
         action = lambda { client.execute('SELECT 1 as [one]').each }
         assert_raise_tinytds_error(action) do |e|
           assert_equal 20003, e.db_error_number
           assert_equal 6, e.severity
           assert_match %r{timed out}i, e.message, 'ignore if non-english test run'
         end
       ensure
-        STDOUT.puts "Reconnect network!"
-        sleep 10
+        docker_container('unpause', wait_for: 1)
         action = lambda { client.execute('SELECT 1 as [one]').each }
         assert_raise_tinytds_error(action) do |e|
           assert_equal 20047, e.db_error_number
-          assert_equal 1, e.severity
+          assert_equal 9, e.severity
           assert_match %r{dead or not enabled}i, e.message, 'ignore if non-english test run'
         end
         close_client(client)

test/test_helper.rb

@@ -246,5 +246,9 @@ def rollback_transaction(client)
       client.execute("ROLLBACK TRANSACTION").do
     end
 
+    def docker_container(cmd, wait_for: 0)
+      system("docker #{cmd} $(docker ps --format '{{.Names}}' --filter 'ancestor=metaskills/mssql-server-linux-tinytds:2017-GA') > /dev/null")
+      sleep(wait_for) if wait_for > 0
+    end
   end
 end