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

all: potential infinite recursions in method recursion/retry due to assumption that SessionPool are always working after ._getSession return 'No Session Found' #2165

Open
odeke-em opened this issue Oct 21, 2024 · 4 comments
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

Throughout this package and in my journey of tracing this package, I've encountered the pattern of this form

nodejs-spanner/src/database.ts

Lines 3608 to 3628 in 2a19ef1

writeAtLeastOnce(
mutations: MutationSet,
optionsOrCallback?: CallOptions | CommitCallback,
callback?: CommitCallback
): void | Promise<CommitResponse> {
const cb =
typeof optionsOrCallback === 'function'
? (optionsOrCallback as CommitCallback)
: callback;
const options =
typeof optionsOrCallback === 'object' && optionsOrCallback
? (optionsOrCallback as CallOptions)
: {};
return startTrace('Database.writeAtLeastOnce', this._traceConfig, span => {
this.pool_.getSession((err, session?, transaction?) => {
if (err && isSessionNotFoundError(err as grpc.ServiceError)) {
span.addEvent('No session available', {
'session.id': session?.id,
});
this.writeAtLeastOnce(mutations, options, cb!);

writeAtLeastOnce( 
   mutations: MutationSet, 
   optionsOrCallback?: CallOptions | CommitCallback, 
   callback?: CommitCallback 
 ): void | Promise<CommitResponse> { 
...
     this.pool_.getSession((err, session?, transaction?) => { 
       if (err && isSessionNotFoundError(err as grpc.ServiceError)) { 
         span.addEvent('No session available', { 
           'session.id': session?.id, 
         }); 
         this.writeAtLeastOnce(mutations, options, cb!); 

but notice that if pool._getSession returns Session Not Found, then that method will call itself an indefinite number of times and that's non-determinism. We've got many such cases.

nodejs-spanner/src/database.ts

Lines 2097 to 2103 in 2a19ef1

if (isSessionNotFoundError(err)) {
span.addEvent('No session available', {
'session.id': session?.id,
});
session!.lastError = err;
this.pool_.release(session!);
this.getSnapshot(options, (err, snapshot) => {

@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
@alkatrivedi
Copy link
Contributor

@odeke-em if there will be Session Not Found Error, we will retry the transaction after getting another session from the session pool. In every try we will be getting 'Session Not Found Error' that's very rare.

@alkatrivedi
Copy link
Contributor

Could you please help me understand in which case it could lead to an issue?

@odeke-em
Copy link
Contributor Author

If for example the pool is faulty, or the Cloud Spanner database is under load or even a billing issue on the account.

@surbhigarg92 surbhigarg92 self-assigned this Nov 6, 2024
@surbhigarg92
Copy link
Contributor

@odeke-em IIUC, in all above cases, sessions will not be created at the time of session pool creation.

In this case there will not be any Session not found error instead , the request will keep waiting for the session to be available and ultimately the request will get timed out. Refer
You could also set the timeout for acquiring sessions by setting SessionPoolOptions:acquireTimeout default is infinity. Refer https://cloud.google.com/blog/topics/developers-practitioners/deploying-cloud-spanner-based-nodejs-application

Please let us know if you are not seeing the above behavior and instead seeing infinite recursion for your use case ?

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

3 participants