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

Database.runTransactionAsync should check that the error returned can be re-tried instead of the indefinite while(true) loop #2166

Closed
odeke-em opened this issue Oct 21, 2024 · 1 comment
Assignees
Labels
api: spanner Issues related to the googleapis/nodejs-spanner API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@odeke-em
Copy link
Contributor

If we examine the code for Database.runTransactionAsync, it invokes a while(true) loop retrying on any error without differentiating if the error can be retried or not and the only way to break out of that loop is by throwing an exception

nodejs-spanner/src/database.ts

Lines 3370 to 3406 in 2a19ef1

while (true) {
try {
const [session, transaction] = await promisify(getSession)();
transaction.requestOptions = Object.assign(
transaction.requestOptions || {},
options.requestOptions
);
if (options.optimisticLock) {
transaction.useOptimisticLock();
}
if (options.excludeTxnFromChangeStreams) {
transaction.excludeTxnFromChangeStreams();
}
sessionId = session?.id;
span.addEvent('Using Session', {'session.id': sessionId});
const runner = new AsyncTransactionRunner<T>(
session,
transaction,
runFn,
options
);
try {
return await runner.run();
} finally {
this.pool_.release(session);
}
} catch (e) {
if (!isSessionNotFoundError(e as ServiceError)) {
span.addEvent('No session available', {
'session.id': sessionId,
});
throw e;
}
}
}
}

The only way I can test out functionality of that code in error is by throwing an exception and there aren't tests for what happens with a variety of errors. Let's please add them.

@odeke-em odeke-em added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Oct 21, 2024
@product-auto-label product-auto-label bot added the api: spanner Issues related to the googleapis/nodejs-spanner API. label Oct 21, 2024
odeke-em added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 21, 2024
Extracted out of PR googleapis#2158, this change traces
Database.runTransactionAsync. However, testing isn't effective
because of bugs such as googleapis#2166.
odeke-em added a commit to odeke-em/nodejs-spanner that referenced this issue Oct 21, 2024
Extracted out of PR googleapis#2158, this change traces
Database.runTransactionAsync. However, testing isn't effective
because of bugs such as googleapis#2166.
@surbhigarg92 surbhigarg92 self-assigned this Nov 7, 2024
@surbhigarg92
Copy link
Contributor

@odeke-em This is by design. This while loop is added for retrying session not found error.

Other errors will not be retried and the error will be thrown from here

Please feel free to re-open the issue, if this doesn't satisfy your error.

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. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

No branches or pull requests

2 participants