properly report stored procedure errors when using info messages

Capturing errors while in a non-blocking state was originally structured to capture a single error. This was intentional in order to avoid capturing more generic info messages that FreeTDS might send before the Global VM Lock was obtained. In most circumstances this is what we want. However, now that we capture info messages it is possible that a info message will be stored and the actual runtime error will be discarded as non-important. The result is that while a runtime error is reported in the database, a TinyTds error is never thrown and only the info message is handled. A subset of this problem is that only one info message can be captured while in non-blocking mode which prevents stored procedures from reporting multiple info messages to TinyTds.

To fix this issue, the reported messages are stored within a dynamic array of tinytds_errordata structs, then processed normally once the GVL is obtained.

Given the fact that we don't know the number of messages that will be sent, we dynamically manage and re-allocate memory for the nonblocking_errors array as needed. We can't use the ruby C API because it is not safe to call while in a non-blocking state as shown by #133.

Author: bvogelzang

ext/tiny_tds/client.c

@@ -57,6 +57,15 @@ VALUE rb_tinytds_raise_error(DBPROCESS *dbproc, int is_message, int cancel, cons
 
 
 // Lib Backend (Memory Management & Handlers)
+static void push_userdata_error(tinytds_client_userdata *userdata, tinytds_errordata error) {
+  // reallocate memory for the array as needed
+  if (userdata->nonblocking_errors_size == userdata->nonblocking_errors_length) {
+    userdata->nonblocking_errors_size *= 2;
+    userdata->nonblocking_errors = realloc(userdata->nonblocking_errors, userdata->nonblocking_errors_size * sizeof(tinytds_errordata));
+  }
+
+  userdata->nonblocking_errors[userdata->nonblocking_errors_length++] = error;
+}
 
 int tinytds_err_handler(DBPROCESS *dbproc, int severity, int dberr, int oserr, char *dberrstr, char *oserrstr) {
   static const char *source = "error";
@@ -111,24 +120,16 @@ int tinytds_err_handler(DBPROCESS *dbproc, int severity, int dberr, int oserr, c
       userdata->dbcancel_sent = 1;
     }
 
-    /*
-    If we've already captured an error message, don't overwrite it. This is
-    here because FreeTDS sends a generic "General SQL Server error" message
-    that will overwrite the real message. This is not normally a problem
-    because a ruby exception is normally thrown and we bail before the
-    generic message can be sent.
-    */
-    if (!userdata->nonblocking_error.is_set) {
-      userdata->nonblocking_error.is_message = 0;
-      userdata->nonblocking_error.cancel = cancel;
-      strncpy(userdata->nonblocking_error.error, dberrstr, ERROR_MSG_SIZE);
-      strncpy(userdata->nonblocking_error.source, source, ERROR_MSG_SIZE);
-      userdata->nonblocking_error.severity = severity;
-      userdata->nonblocking_error.dberr = dberr;
-      userdata->nonblocking_error.oserr = oserr;
-      userdata->nonblocking_error.is_set = 1;
-    }
-
+    tinytds_errordata error = {
+      .is_message = 0,
+      .cancel = cancel,
+      .severity = severity,
+      .dberr = dberr,
+      .oserr = oserr
+    };
+    strncpy(error.error, dberrstr, ERROR_MSG_SIZE);
+    strncpy(error.source, source, ERROR_MSG_SIZE);
+    push_userdata_error(userdata, error);
   } else {
     rb_tinytds_raise_error(dbproc, 0, cancel, dberrstr, source, severity, dberr, oserr);
   }
@@ -144,16 +145,21 @@ int tinytds_msg_handler(DBPROCESS *dbproc, DBINT msgno, int msgstate, int severi
 
   // See tinytds_err_handler() for info about why we do this
   if (userdata && userdata->nonblocking) {
-    if (!userdata->nonblocking_error.is_set) {
-      userdata->nonblocking_error.is_message = !is_message_an_error;
-      userdata->nonblocking_error.cancel = is_message_an_error;
-      strncpy(userdata->nonblocking_error.error, msgtext, ERROR_MSG_SIZE);
-      strncpy(userdata->nonblocking_error.source, source, ERROR_MSG_SIZE);
-      userdata->nonblocking_error.severity = severity;
-      userdata->nonblocking_error.dberr = msgno;
-      userdata->nonblocking_error.oserr = msgstate;
-      userdata->nonblocking_error.is_set = 1;
-    }
+    /*
+    In the case of stored procedure execution we can receive multiple messages
+    (including errors). We keep track of those here so they can be processed
+    once the blocking call returns.
+    */
+    tinytds_errordata error = {
+      .is_message = !is_message_an_error,
+      .cancel = is_message_an_error,
+      .severity = severity,
+      .dberr = msgno,
+      .oserr = msgstate
+    };
+    strncpy(error.error, msgtext, ERROR_MSG_SIZE);
+    strncpy(error.source, source, ERROR_MSG_SIZE);
+    push_userdata_error(userdata, error);
 
     if (is_message_an_error && !dbdead(dbproc) && !userdata->closed) {
       dbcancel(dbproc);
@@ -171,7 +177,12 @@ static void rb_tinytds_client_reset_userdata(tinytds_client_userdata *userdata)
   userdata->dbsqlok_sent = 0;
   userdata->dbcancel_sent = 0;
   userdata->nonblocking = 0;
-  userdata->nonblocking_error.is_set = 0;
+  userdata->nonblocking_errors_length = 0;
+  if (userdata->nonblocking_errors_size) {
+    free(userdata->nonblocking_errors);
+  }
+  userdata->nonblocking_errors = malloc(ERRORS_STACK_INIT_SIZE * sizeof(tinytds_errordata));
+  userdata->nonblocking_errors_size = ERRORS_STACK_INIT_SIZE;
 }
 
 static void rb_tinytds_client_mark(void *ptr) {
@@ -190,7 +201,9 @@ static void rb_tinytds_client_free(void *ptr) {
     cwrap->closed = 1;
     cwrap->userdata->closed = 1;
   }
-  xfree(cwrap->userdata);
+  if (cwrap->userdata->nonblocking_errors_size) {
+    free(cwrap->userdata->nonblocking_errors);
+  }
   xfree(ptr);
 }
 

ext/tiny_tds/client.h

@@ -5,9 +5,9 @@
 void init_tinytds_client();
 
 #define ERROR_MSG_SIZE 1024
+#define ERRORS_STACK_INIT_SIZE 2
 
 typedef struct {
-  short int is_set;
   int is_message;
   int cancel;
   char error[ERROR_MSG_SIZE];
@@ -25,7 +25,9 @@ typedef struct {
   RETCODE dbsqlok_retcode;
   short int dbcancel_sent;
   short int nonblocking;
-  tinytds_errordata nonblocking_error;
+  short int nonblocking_errors_length;
+  short int nonblocking_errors_size;
+  tinytds_errordata *nonblocking_errors;
   VALUE message_handler;
 } tinytds_client_userdata;
 

ext/tiny_tds/result.c

@@ -95,17 +95,20 @@ static void nogvl_cleanup(DBPROCESS *client) {
   Now that the blocking operation is done, we can finally throw any
   exceptions based on errors from SQL Server.
   */
-  if (userdata->nonblocking_error.is_set) {
-    userdata->nonblocking_error.is_set = 0;
+  for (short int i = 0; i < userdata->nonblocking_errors_length; i++) {
+    tinytds_errordata error = userdata->nonblocking_errors[i];
     rb_tinytds_raise_error(client,
-      userdata->nonblocking_error.is_message,
-      userdata->nonblocking_error.cancel,
-      userdata->nonblocking_error.error,
-      userdata->nonblocking_error.source,
-      userdata->nonblocking_error.severity,
-      userdata->nonblocking_error.dberr,
-      userdata->nonblocking_error.oserr);
+      error.is_message,
+      error.cancel,
+      error.error,
+      error.source,
+      error.severity,
+      error.dberr,
+      error.oserr
+    );
   }
+
+  userdata->nonblocking_errors_length = 0;
 }
 
 static RETCODE nogvl_dbsqlok(DBPROCESS *client) {

test/result_test.rb

@@ -652,6 +652,18 @@ class ResultTest < TinyTds::TestCase
             assert_equal 1, messages.length, 'there should be one message after one print statement'
             assert_equal msg, m.message, 'message text'
           end
+
+          it 'must raise an error preceded by a `print` message' do
+            messages.clear
+            action = lambda { @client.execute("EXEC tinytds_TestPrintWithError").do }
+            assert_raise_tinytds_error(action) do |e|
+              assert_equal 'hello', messages.first.message, 'message text'
+
+              assert_equal "Error following print", e.message
+              assert_equal 16, e.severity
+              assert_equal 50000, e.db_error_number
+            end
+          end
         end
 
         it 'must not raise an error when severity is 10 or less' do
@@ -770,4 +782,3 @@ def insert_and_select_datatype(datatype)
   end
 
 end
-

test/test_helper.rb

@@ -153,6 +153,7 @@ def load_current_schema
       loader.execute(drop_sql).do
       loader.execute(schema_sql).do
       loader.execute(sp_sql).do
+      loader.execute(sp_error_sql).do
       loader.close
       true
     end
@@ -167,7 +168,10 @@ def drop_sql_sybase
         ) DROP TABLE datatypes
         IF EXISTS(
           SELECT 1 FROM sysobjects WHERE type = 'P' AND name = 'tinytds_TestReturnCodes'
-        ) DROP PROCEDURE tinytds_TestReturnCodes|
+        ) DROP PROCEDURE tinytds_TestReturnCodes
+        IF EXISTS(
+          SELECT 1 FROM sysobjects WHERE type = 'P' AND name = 'tinytds_TestPrintWithError'
+        ) DROP PROCEDURE tinytds_TestPrintWithError|
     end
 
     def drop_sql_microsoft
@@ -181,7 +185,11 @@ def drop_sql_microsoft
         IF EXISTS (
           SELECT name FROM sysobjects
           WHERE name = 'tinytds_TestReturnCodes' AND type = 'P'
-        ) DROP PROCEDURE tinytds_TestReturnCodes|
+        ) DROP PROCEDURE tinytds_TestReturnCodes
+        IF EXISTS (
+          SELECT name FROM sysobjects
+          WHERE name = 'tinytds_TestPrintWithError' AND type = 'P'
+        ) DROP PROCEDURE tinytds_TestPrintWithError|
     end
 
     def sp_sql
@@ -191,6 +199,13 @@ def sp_sql
         RETURN(420) |
     end
 
+    def sp_error_sql
+      %|CREATE PROCEDURE tinytds_TestPrintWithError
+        AS
+        PRINT 'hello'
+        RAISERROR('Error following print', 16, 1)|
+    end
+
     def find_value(id, column, query_options={})
       query_options[:timezone] ||= :utc
       sql = "SELECT [#{column}] FROM [datatypes] WHERE [id] = #{id}"
@@ -214,4 +229,3 @@ def rollback_transaction(client)
 
   end
 end
-