Fix transaction support and add new Connection#transaction helper.

[arthurschreiber]

This adds a new transaction helper that makes the use of transactions a
breeze. It uses a mix of transactions and transaction savepoints to
enable correctly nested transactions, while giving the user the ability
to only roll back the contents of specific transaction blocks.

This is a potential breakage for existing users that make use of
Connection#beginTransaction, Connection#rollbackTransaction or
Connection#commitTransaction, but these never worked correctly in the
first place.

Fixes #152

---

As described in #152, the use of nested transactions is super broken in tedious in it's current form.

This PR adds Connection#saveTransaction to create transaction savepoints and uses that to build a Connection#transaction helper that correctly employs transactions and savepoints to implement sane nested transaction handling.

Example:

// create a user with the given email address, and call
// `cb` either with an error or null and the new user's id.
function createUser(name, emailAddress, cb) {
  connection.transaction(function(err, done) {
    // If starting the transaction failed, we can just stop
    if (err) return cb(err);

    var id;
    var sql = "INSERT INTO users (name) OUTPUT INSERTED.id VALUES (@0)";

    var request = new tedious.Request(sql, function(err) {
      // if the request failed, we need to rollback the transaction
      // and notify the calling code.
      if (err) return done(err, cb);

      createEmail(id, emailAddress, function(err) {
        // The continuation given by Connection#transaction can be passed
        // a potential error and the "next" callback to be executed.
        // Additionally, it can take more arguments to pass down to the 
        // callback.
        done(err, cb, id);
        // This is the same as:
        // done(err, function(err) { cb(err, id); })
      });
    });
    request.on('row', function(row) { id = row[0].value; });

    request.addParameter(0, tedious.TYPES.NVarChar, name);
    connection.execSql(request);
  });
}

function createEmail(userId, address, cb) {
  var sql = "INSERT INTO email_addresses (user_id, address) VALUES (@0, @1)";
  var request = new tedious.Request(sql, cb);
  request.addParameter(0, tedious.TYPES.Int, userId);
  request.addParameter(0, tedious.TYPES.NVarChar, address);
  connection.execSql(request);
}

// ...

// Calling code does not need to know that transactions are used.
createUser("Arthur", "arthur@example.com", function(err, id) {
  // ...
});

[arthurschreiber]

This fixes one of my major gripes that I had with tedious so far. I'd love to hear back what other people think who actually use or tried to use transactions with tedious.

[bretcope]

I don't have a problem with the idea. Could you describe what you mean by possible breaking changes? Does that apply strictly to users who did nested transactions before? If so, how will this break that?

[arthurschreiber]

Let me first describe the changes I made to some of the existing methods, and then I'll try to explain what I mean by "potentially breaking changes".

I removed the Connection internal #transactions array that basically worked like a transaction stack: Each time #beginTransaction was called, a new Transaction object was pushed onto this stack, and when #rollbackTransaction was called, a transaction was popped off this stack, and the name of this transaction was used to identify the transaction to be rolled back.

Now, this never worked correctly, because nested transactions don't work like a stack in MS SQL. Here's a snippet from the documentation:

Naming multiple transactions in a series of nested transactions with a transaction name has little effect on the transaction. Only the first (outermost) transaction name is registered with the system. A rollback to any other name (other than a valid savepoint name) generates an error. None of the statements executed before the rollback is, in fact, rolled back at the time this error occurs. The statements are rolled back only when the outer transaction is rolled back.

So, if you had nested transactions using tedious, and called #rollbackTransaction, this would a) raise an error and b) leave your connection in a weird state. This basically made nested transactions unusable, because your code would need to know whether it runs inside a nested transaction or not and depending on that, it would have to behave differently to ensure that it does not call #rollbackTransaction.

I changed #rollbackTransaction and #commitTransaction to optionally take a transaction name parameter.

This means that

• #beginTransaction starts a new transaction (either named or unnamed). Nothing changed here.
• #rollbackTransaction rolls back the outermost transaction (effectively rolling back all running transactions) if no name given, and does no longer raise an error if you're inside a nested transaction. This now reflects executing ROLLBACK TRANSACTION, effectively being less confusing. If you pass it a name, it will either rollback all running transactions (if the given name is the name of the currently executing outer transaction), or it will roll back to the savepoint that matches the given name. If there is no savepoint with the given name or the name does not match the outermost transaction, this will raise an error. This mirrors executing ROLLBACK TRANSACTION <name>, again, being less confusing.
• #commitTransaction without a name will commit the currently executing transaction, or with a name, it will commit the named transaction.

So, the potential breaking change would be that #rollbackTransaction is no longer "totally broken", and that it will no longer raise an error when executing inside a nested transaction, but will effectively rollback all running transactions.

As you can see, none of the tedious integration tests were touched, so if your current code does "work", it will continue to work. Still there is a potential for backwards incompatibility if you somehow depend on your code to be broken. 😧

Again, all these changes make these methods work like what one would expect.

On top of that, #transaction gives you an abstraction that allows you to stop worrying about whether your transaction creating code is going to run inside a transaction or not. It will "just work".

I hope that answers your question. 😄

[arthurschreiber]

On another note, I'll extend Connection#reset to reset all open transactions and reset the internal openTransactions counter to 0. This is another issue right now, that is really painful. If you use connection pooling and your connection is somehow stuck with an open transaction, you have to manually make sure you rollback all running transactions. Connection#reset should be doing that for you.

[bretcope]

Sounds good to me.

[arthurschreiber]

Ok, great. I'll try to wrap this up tomorrow.

[mbroadst]

really looking forward to this making it in!

[arthurschreiber]

So, this is basically ready and can be merged/released.

@mbroadst Did you have any chance to take this on a test ride?

[nsilberman]

great news !

[mbroadst]

@arthurschreiber I haven't test driven it yet, but we use tedious in sequelize and I can absolutely verify that transaction support is pretty broken in its current incarnation. Looking forward to giving this a shot in the next couple of days (as well as a few of your other PRs, like the stream based tokenizing). Thanks for the good work

[nsilberman]

Can't wait to see the PR included in the master :)

[mbroadst]

@arthurschreiber aren't you a collaborator? Just merge away my man 😄

[arthurschreiber]

@mbroadst I am, but just being a collaborator does not mean that I'll merge things willy-nilly. 😉

One more issue came up:

Connection#transaction does not yet support a transactionlevel option. Adding the option itself is easy, but it's not clear how it should work in the case of nested transactions.

Example:

connection.transaction(function() {
  // Outer transaction has tedious.ISOLATION_LEVEL.READ_UNCOMMITTED

  // Suppose doSomethingElse calls connection.transaction
  // with tedious.ISOLATION_LEVEL.READ_COMMITTED
  doSomethingElse(connection, function() {

    // All queries executed here will be run with
    // tedious.ISOLATION_LEVEL.READ_COMMITTED,
    // even though they're run after the inner transaction
    // finished.
  });


}, tedious.ISOLATION_LEVEL.READ_UNCOMMITTED);

Are we just going to say that whatever isolation level you set last is the one that is going to be valid until all transactions finished?

[arthurschreiber]

On another note, Rails' ActiveRecord (which this feature is inspired from) raises an error if you try to set the isolation level on a nested transaction. Maybe we should do that too?

[arthurschreiber]

One more issue: If you run into a batch aborting error (e.g. you try to create an already existing table inside a transaction), any rollback requests will fail (because the transaction was already rolled back). My idea here is to simply ignore these errors. The transaction is already rolled back, and there's nothing left for us to do.

This also means that you should not try to "catch" RequestErrors coming from the connection helper, and treat transactions as "poisoned" if such an error is raised somewhere during the course of a transaction.

[bretcope]

I agree with your last two comments. I think they fit within the principle of least-surprise.

[arthurschreiber]

I just squashed and pushed some additional changes:

• The Connection#transaction helper now works even if the outermost transaction was started with #beginTransaction. It will also work correctly now if somehow the outermost transaction was rolled back. Basically, it now does the correct thing (:tm:) no matter how transactions are started or rolled back.
• Setting the isolation level works correctly now, and it works like you would expect: It sets the isolation level to be used for the rest of the connection's lifetime, just like calling Connection#beginTransaction does. To be honest, I think the behavior of sqlserver is just crazy, but that's how it is.
• I replaced the .openTransactions counter property with a simple .inTransaction boolean property, that allows checking whether a connection currently has a transaction going on.

I'll wait for the build to finish and will get this merged then. ✨

[arthurschreiber]

Let's 🚢 it!

[arthurschreiber]

I released this as 1.9.0.

[arthurschreiber]

@mbroadst @nsilberman Any success with using this, so far?