Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AsyncTransactionManager needs to roll back active transaction when closed #504

Closed
elefeint opened this issue Oct 9, 2020 · 1 comment · Fixed by #505
Closed

AsyncTransactionManager needs to roll back active transaction when closed #504

elefeint opened this issue Oct 9, 2020 · 1 comment · Fixed by #505
Assignees
Labels
api: spanner priority: p2 type: bug

Comments

@elefeint
Copy link
Contributor

@elefeint elefeint commented Oct 9, 2020

When testing the new R2DBC driver with the Cloud Spanner emulator, I noticed that the active transaction was not being rolled back despite AsyncTransactionManager.close() being called.

The contract for close() is to roll back the active transaction when closed, but I wonder if close() was not meant to be implemented as closeAsync() and kind of got left out.

Environment details

Spanner client library version: 1.59.0

Steps to reproduce

  1. Start emulator and set the emulator environment variable (SPANNER_EMULATOR_HOST=localhost:9010). The issue has nothing to do with the emulator, but it's really easy to observe when using one.
  2. Update "project", "instance", "database" to real values.
  3. Set up the following table definition:
    gcloud spanner databases ddl update database --ddl="CREATE TABLE test ( value INT64 ) PRIMARY KEY (value)" --instance=instance
  4. Run transactionManagerRollsBackTransactionWhenClosed twice; observe that everything works as expected.
  5. Run asyncTransactionManagerDoesNotRollBackTransactionWhenClosed twice; observe that the first run succeeds, but the second fails with error io.grpc.StatusRuntimeException: ABORTED: Transaction NNN aborted due to active transaction MMM. The emulator only supports one transaction at a time.
  DatabaseId databaseId = DatabaseId.of("project", "instance", "database");
  SpannerOptions options = SpannerOptions.newBuilder().build();
  Spanner spanner = options.getService();
  DatabaseClient dbClient = null;
  TransactionManager txnManager = null;
  AsyncTransactionManager asyncTxnManager = null;

  @Test
  public void transactionManagerRollsBackTransactionWhenClosed() throws Exception {

    try {
      dbClient = spanner.getDatabaseClient(databaseId);
      txnManager = dbClient.transactionManager();
      TransactionContext ctx = txnManager.begin();
      ctx.executeUpdate(Statement.newBuilder("INSERT INTO test (value) VALUES (1234567)").build());

      // MISSING COMMIT; expecting rollback
      // tm.commit();

    } finally {

      if (txnManager != null) {
        System.out.println("************ CLOSING TRANSACTION MANAGER");
        txnManager.close();
      }
    }
  }

  @Test
  public void asyncTransactionManagerDoesNotRollBackTransactionWhenClosed() throws Exception {

    try {
      dbClient = spanner.getDatabaseClient(databaseId);
      asyncTxnManager = dbClient.transactionManagerAsync();
      TransactionContext ctx = asyncTxnManager.beginAsync().get();
      ctx.executeUpdateAsync(Statement.newBuilder("INSERT INTO test (value) VALUES (1234567)").build())
        .get();

      // MISSING COMMIT; expecting rollback upon transaction manager closing
      // tm.commit();

    } finally {

      if (asyncTxnManager != null) {
        asyncTxnManager.close();
      }
    }
  }
@product-auto-label product-auto-label bot added the api: spanner label Oct 9, 2020
@hengfengli hengfengli added the triage me label Oct 9, 2020
@olavloite olavloite added type: bug and removed triage me labels Oct 9, 2020
olavloite added a commit that referenced this issue Oct 9, 2020
AsyncTransctionManager should rollback the transaction if close is called
while the transaction is still in state STARTED. Failing to do so, will
keep the transaction open on the backend for longer than necessary, holding
on to locks until it is garbage collected after 10 seconds.

Fixes #504
@olavloite
Copy link
Contributor

@olavloite olavloite commented Oct 9, 2020

@elefeint Thanks for the detailed report and easy to reproduce example. I've provided a fix for it in #505.

@yoshi-automation yoshi-automation added the triage me label Oct 9, 2020
@skuruppu skuruppu added priority: p2 and removed triage me labels Oct 13, 2020
olavloite added a commit that referenced this issue Oct 14, 2020
* fix: AsyncTransactionManager should rollback on close

AsyncTransctionManager should rollback the transaction if close is called
while the transaction is still in state STARTED. Failing to do so, will
keep the transaction open on the backend for longer than necessary, holding
on to locks until it is garbage collected after 10 seconds.

Fixes #504

* feat: return rollback result from close

* fix: add ignored diff to clirr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner priority: p2 type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants