Datastore transactions do not work without context manager

[Bogdanp]

The docs state that you can use transactions without a context manager, but this is not exactly true because you need to resort to some private methods on Client:

• this file uses the context manager and produces correct output
• this file uses transactions as described by the aforementioned docs and fails the assertion
• this file uses transactions along with calls to client._push_batch and client._pop_batch and produces correct output

The issue is evident in Client.get_multi and Client.put_multi where transaction and current are None unless a Batch or Transaction has been pushed to the _batch_stack and the only time that happens is when you __enter__ a Batch.

[daspecster]

@Bogdanp thanks for finding this!

In the case where the assertions fail, is it both values or just one of them that isn't getting set when you t.commit()?

[Bogdanp]

Both values are updated. However, due to the fact that there is no transaction id "in scope" for those get and put requests, the serializability of the operations is not enforced.

Here's some sample output:

(tempenv-3357153745070) ~ [1]> env DATASTORE_DATASET="test" DATASTORE_PROJECT_ID="test" DATASTORE_HOST="http://127.0.0.1:9999" python failing_test.py 
60
40
Traceback (most recent call last):
  File "failing_test.py", line 54, in <module>
    assert a1["balance"] == 100 - n * 10
AssertionError

I just added a couple of print statements before the assertions in the failing example:

    print a1["balance"]
    print a2["balance"]

    assert a1["balance"] == 100 - n * 10
    assert a2["balance"] == n * 10

[tseaver]

Client.put_multi is never called in your failing example: the changes are all accumulated inside the transaction's _commit_request protobuf. I think the error is that the transaction ID (fetched during Transaction.begin) is not being stored into that protobuf.

[tseaver]

I just tried to write a system test replicating your failing report, and found that it passes:

    def test_transaction_via_imperative_begin_commit(self):
        BEFORE_1 = 100
        BEFORE_2 = 0
        TRANSFER_AMOUNT = 40
        key1 = Config.CLIENT.key('account', '123')
        account1 = datastore.Entity(key=key1)
        account1['balance'] = BEFORE_1
        key2 = Config.CLIENT.key('account', '234')
        account2 = datastore.Entity(key=key2)
        account2['balance'] = BEFORE_2
        Config.CLIENT.put_multi([account1, account2])

        def transfer_funds(client, from_key, to_key, amount):
            xact = client.transaction()
            xact.begin()
            from_account = client.get(from_key)
            to_account = client.get(to_key)
            from_account['balance'] -= amount
            to_account['balance'] += amount

            xact.put(from_account)
            xact.put(to_account)
            xact.commit()

        transfer_funds(Config.CLIENT, key1, key2, TRANSFER_AMOUNT)

        after1 = Config.CLIENT.get(key1)
        after2 = Config.CLIENT.get(key2)
        self.assertEqual(after1['balance'], BEFORE_1 - TRANSFER_AMOUNT)
        self.assertEqual(after2['balance'], BEFORE_2 + TRANSFER_AMOUNT)

One issue might be thatClient.get_multi is not guaranteed to return the items in the order of the requested keys: can you update your failing example to compensate (e.g., either use Client.get or else check the keys of the returned entities)?

[tseaver]

(Of course, my example is not threaded with multiple writers, so it is not really a good enough replica).

[Bogdanp]

Here's a failing example w/o any get_multi calls.

(tempenv-3357153745070) ~ [1]> env DATASTORE_DATASET="test" DATASTORE_PROJECT_ID="test" DATASTORE_HOST="http://127.0.0.1:9999" python failing_test.py 
60
40
Traceback (most recent call last):
  File "failing_test.py", line 56, in <module>
    assert a1["balance"] == 100 - n * 10
AssertionError

BTW, I picked this example specifically because it's what's being used in the official docs (note the use of get_multi).

[Bogdanp]

Are you sure this isn't the culprit? transaction_id ends up being None there unless _push_batch is called prior to get/get_multi.

[tseaver]

I was still investigating: you are correct that get/get_multi do not provide any support for doing the reads in the context of an explicit transaction. I have a PR in progress which will add that support.

[tseaver]

@Bogdanp Thanks for the detailed bug report! I was able to get a version of your failing script passing on the branch for #1861 (after failing on master in exactly the way you describe here).