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

src/transction:fix Snapshot.createReadStream on error this.begin() invocation when no transactionID is detected and it was not aborted #2170

Closed
odeke-em opened this issue Oct 22, 2024 · 1 comment
Assignees
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API.

Comments

@odeke-em
Copy link
Contributor

As a consequence of this problem, in my observability changes I have to invoke await this.begin() so as to ensure proper span production but @surbhigarg raised a point in a code review with

Can we hold on this change of making 'error' event async. I know this would mean that our few spans wont work correctly.
I am working on another PR to see if this check can be removed #2168

Adding async here will cause other issues like in below when runTransactionAsync method tries to insert an invalid error, normally this error event is invoked first and then catch block. But if you add async here, this would make catch block invoked first as error event is called asynchronously which will cause other issues.

try {
    await database.runTransactionAsync(async transaction => {
      await transaction.run(
        "INSERT INTO Singers (SingerId, FirstName) VALUES (310, 'abc')"
      );
      await transaction.commit();
    });
    await database.runTransactionAsync(async transaction => {
      await transaction.run(
        "INSERT INTO Singers (SingerId, FirstName) VALUES (310, 'abc')"
      );
      await transaction.commit();
    });
  } catch (err) {
    console.error('ERROR:', err);
    // throw err;
  } finally {
    database.close();
  }

Remove this change and add a bug where we mention what scenarios are not working and I will take it up separately.

Originally posted by @surbhigarg92 in #2158 (comment)

@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Oct 22, 2024
@surbhigarg92 surbhigarg92 self-assigned this Nov 6, 2024
@surbhigarg92
Copy link
Contributor

@odeke-em IMU this issue is duplicate of other issue raised #2159

this.begin is invoked for non-aborted errors in createReadStream and runStream

Closing this bug . Pleas feel free to re-open if you feel otherwise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API.
Projects
None yet
Development

No branches or pull requests

2 participants