transaction.rollback() fails after sql error generated by msnodesql driver

[shandyba]

Hi,

today I spotted a nasty issue with Transaction interface of node-mssql not working as expected under the following conditions.

Driver used: msnodesql (0.2.1)

1. Connect to an sql server.
2. Begin a transaction.
3. Make any sql query resulting in error (for example, violate a db constraint with an insert statement).
4. Inside error callback or error event listener (if streaming) attempt to rollback a transaction.

Result: Error rolling back transaction with message "TransactionError: Can't rollback transaction. There is a request in progress." + all rows affected by that transaction remain locked (last statement should be investigated better).
Expected result: transaction gets rolled back correctly.

Diving a bit into this I found a cause, which, unfortunately, seems to be located in msnodesql's code:

msnodesql, after emitting an error event does not emit done event (as it does for positive execution cases).
Thus, mssql has no chance to release a connection utilized by the call causing an sql error and, subsequently, does not mark transaction as _working = false;, which leads to all subsequent attempts to perform queries on that transactions to get queued and transaction operations to always complete with the abovementioned error message.

I was able to workaround the case by applying the following simple patch to msnodesql's sources:

diff -r 6891fd1dd238 node_modules/msnodesql/lib/sql.js
--- a/node_modules/msnodesql/lib/sql.js     Mon Sep 15 18:19:12 2014 +0300
+++ b/node_modules/msnodesql/lib/sql.js     Tue Sep 16 16:40:27 2014 +0300
@@ -33,6 +33,7 @@
     }
     else if (notify && notify.listeners('error').length > 0) {
         notify.emit('error', err);
+        notify.emit('done');
     }
     else {
         throw new Error(err);

I.e. by emitting a done event after an error one. Unfortunately, I'm not sure this will work correctly in all occasions, but at least I was able to proceed without any noticeable glitches (yet) in my (rather simple) scenario.

I understand that it is very likely that it will be confirmed that this issue resides in msnodesql's code (and should be addressed there), but, as we all understand, it looks pretty unlikely it will get fixed any soon.

So, I think if workarounding this case on mssql's side is not an option, mentioning this as a known issue might at least save time for others having this pain.

[patriksimek]

Thanks for the report, I have just updated known issues with that.

[praneetkinnser]

I am facing a similar issue with Tedious too, but I think we can fix this in mssql.

<begin>

request1 
if(err)-> <rollback>
else->
    request2
    if(err)-> <rollback>    
    else->
        request3
        if(err)-> <rollback>
        else->
            <commit>        

I am nesting few requests (within callbacks, where each request executes a stored procedure) on the same transaction. Everything works fine, until processing one of those requests results in an uncaught exception. For example, trying to insert a 50 char word in a size 10 column. In such an event, when I try to rollback the transaction, it gives the same error: "TransactionError: Can't rollback transaction. There is a request in progress."

main.js:

Transaction.prototype.rollback = function(callback) {
     ...
      if (this._working) {
        if (typeof callback === "function") {
          callback(new TransactionError("Can't rollback transaction. There is a request in progress.", 'EREQINPROG'));
        }
        return this;
      }
     ...
    };

This happens because when trying to rollback, the _working is still set to true. The only place place _working is being set to false is in next() when queue is empty. Practically, I would like to rollback a transaction in case of any error. These requests aren't "pending" in the true sense because their callback has already been called. Shouldn't the rollback function flush the pending/queued requests, and set the working flag to false?

In case of commit I agree that all requests should be complete before committing the transaction, but if we want to undo all requests, why wait for queued requests?

Please do let me know if you can think of another solution for this. Thanks!

[patriksimek]

Thanks for the report, it's fixed and will be contained in 2.0 release.

[dhye360]

I'm using 2.3.1 and I still see this issue.
Within a transaction a request fails because the field isn't large enough; but the rollback fails. It does not crash my Node server, so what state is the system in at this point? One of the connection pool is consumed and stuck?

I apologize for the limited information, but any insight on what I should expect with a transaction would help.

Thanks

[Doc999tor]

I'm still facing the original issue - transaction that has to fail (foreign key in my example) gets stuck

await transaction.begin()
try {
	const stmt = new PreparedStatement(transaction);
	stmt.input('client_id', Int);
	await stmt.prepare(`INSERT INTO clients values (@client_id)`);
	await stmt.execute({ client_id }); // fails on foreign key constraint but gets stuck
	await stmt.unprepare();
	await transaction.commit()
} catch (e) { ... /* nothing happens */ }

"msnodesqlv8": "2.0.6",
"mssql": "6.2.3",

[Doc999tor]

The option to emit done didn't help - TimelordUK/node-sqlserver-v8#183
The transaction just passes and commits - no rollback