Inaccessible transaction.rollback

[stephenplusplus]

From https://github.com/GoogleCloudPlatform/gcloud-node/pull/627/files#r31352472

Question 1

A rollback can only be done on a transaction that has been committed (I think?), but that's a problem:

dataset.runInTransaction(function(transaction, commit) {
  commit();
}, function(err) {
  // no `transaction` here to call `rollback` on.
});

An option would be to remove the last callback.

dataset.runInTransaction(function(transaction, commit) {
  commit(function(err) {
    if (err) {
      transaction.rollback();
    }
  });
});

Question 2

Is there a use case for rolling back a transaction that wasn't just created? We currently don't support this.

[stephenplusplus]

I think we need to allow a way to get a Transaction object with the usual accessor method:

(using datastore v1beta3 as an example)

var transaction = datastore.transaction('transaction-id');
transaction.rollback(function(err, apiResponse) {});

This solves the second problem. But maybe it can help out with the first if we lose "runInTransaction" and just have "transaction.run"?

var transaction = datastore.transaction(); // no args, it's a new one

transaction.run(function(err) {
  transaction.get(['...'], function(err) {
    if (err) {
      return;
    }

    transaction.commit(function(err) {
      if (err) {
        return;
      }

      // If you want to rollback:
      transaction.rollback(function(err) {});
    });
  });
});

I think it's more clear to remove the magic argument "done()" (which maps to commit) as well as the second callback that went to runInTransaction as it's not totally clear when that gets called. Instead of that way, this style allows for explicit calling of the relevant functions.

[stephenplusplus]

@callmehiphop any thoughts on this idea?

[callmehiphop]

I like it! It feels more consistent with our current APIs.

[callmehiphop]

My only comment would be all the nesting, but if and when we switch to promises, I think it'll actually be really nice..

datastore
  .transaction()
  .run(function() {
    return transaction.get(['...']);
  })
  .then(function() {
    return transaction.commit();
  })
  .then(function() {
    return transaction.rollback();
  });

[stephenplusplus]

👍

[jasonswearingen]

from my own testing, it looks like the logic being discussed here isn't actually how done() and rollback() work?

here is my example code:

    // Save data to your dataset.
    var blogPostData = {
        title: 'How to make the perfect homemade pasta_insert_new_rewrite try get!',
        author: 'Andrew Chilton',
        isDraft: true
    };

    var blogPostKey = dataset.key(['BlogPost', "uniqueKey"]);

    dataset.runInTransaction((transaction, done) => {

        transaction.get(blogPostKey, function (err, entity) {
            console.log("xact get", { err, entity, arguments });

            //transaction.save({
            //    key: blogPostKey,
            //    //method: "insert",
            //    data: blogPostData,
            //}, undefined);
            blogPostData.isDraft = false;

            transaction.save({
                key: blogPostKey,
                //method: "update",
                data: blogPostData,
            }, undefined);


            done();
            if (err != null || entity == null) {
                transaction.rollback(function (err, entity) {
                    console.log("xact rollback", { err, entity, arguments });
                });
            }

        });

    }, function (err, apiResponse) {
        console.log("xact insert finish", { err, apiResponse });
    });

From my various combinations of tests, it seems to behave how I would naturally expect it to : you can only rollback() if you haven't called done() yet, and calling done() after an error or a rollback doesn't work.

[stephenplusplus]

A Transaction only makes a total of 2 requests to the Google API:

1. Begin a transaction: https://cloud.google.com/datastore/docs/apis/v1beta2/datasets/beginTransaction
2. Commit the transaction: https://cloud.google.com/datastore/docs/apis/v1beta2/datasets/commit

A transaction doesn't commit until done() is called. Every operation you make with delete and save is queued until that call.

So in other words, until commit is called, there's nothing to rollback.

Transactions have a maximum duration of 60 seconds with a 10 second idle expiration time after 30 seconds.

• https://cloud.google.com/datastore/docs/concepts/transactions

[jasonswearingen]

hmmm, actually i retried my example code above and it works like what you said.... strange... I could have swore it wasn't working properly when i checked last time!

that said, given my simple example, you can also call rollback() before done() and it still behaves correctly (rolling back the transaction)

A Transaction only makes a total of 2 requests to the Google API:

I can see how it only sets some state twice (creating the transaction and actually writing all the changes) but transactions must round-trip other stuff like transaction.get() requests.

I am guessing (since in my other issue, atomic increments work!) that the transaction verifies the entity version I retrieved via transaction.get() is still the current version before committing the transaction... if so, that's a pretty interesting performance implication there!

by the way, I just tested, and you _can not rollback the transaction in the final callback_ such as shown below. the transaction is already done and can not be rolled back at that point:

////////////////////
///// Example showing that you can not rollback the transaction in the final callback.

    var blogPostData = {
        title: 'rollback in final callback!',
        author: 'a guy',
        isDraft: true
    };

    var blogPostKey = dataset.key(['BlogPost', "uniqueKey"]);

    var _transaction;
    dataset.runInTransaction((transaction, done) => {
        _transaction = transaction;

        transaction.save({
            key: blogPostKey,
            //method: "update",
            data: blogPostData,
        });

        done();

    }, function (err, apiResponse) {
        console.log("xact insert finish", { err, apiResponse });
        _transaction.rollback(function (err, entity) {
            console.log("xact rollback", { err, entity, arguments });
        });
    });

[stephenplusplus]

@pcostell can you fill us in on how are rolling back a transaction is meant to work?

[pcostell]

@stephenplusplus can you clarify what the exact question is?

Some general information:
Right now, Datastore uses optimistic concurrency control. This means that rolling back doesn't do much (it cleans up some state about your transaction, but it isn't strictly necessary). However, we will be adding new types of transaction options and eventually switching to a new backing store which requires locking. This means the rollback is necessary to release the locks, failing to rollback would have an impact on throughput.

[stephenplusplus]

Okay, I was thinking rolling back a transaction is an undo of whatever occurred during the transaction. If it's more of a clean-up, is that something our library should just do automatically after a transactional commit?

[pcostell]

Transaction code should always look something like (excuse the python pseudocode):

try:
  begin_transaction()
  do some stuff, throw error if you want to exit
  commit()
except:
  rollback()

In general, we should be able to hide this in the clients. However, the user needs to be able to say "actually this commit is problematic, abort". Perhaps passing an error into done()?

For example, in the case of a bank transaction, you might read both accounts, and if the source account doesn't have the necessary funds you would rollback, rather than commit the transfer of funds. Right now it doesn't really matter if you rollback or not, but that is purely a Datastore implementation detail. You should approach the problem assuming that the transaction locks all of your reads, so failing to rollback would cause all transactions afterwards on those entities to fail due to contention until the transaction times out.

[stephenplusplus]

That makes a lot of sense, thanks for the explanation.