googleapis / nodejs-spanner

Node.js client for Google Cloud Spanner: the world’s first fully managed relational database service to offer both strong consistency and horizontal scalability.
https://cloud.google.com/spanner/
Apache License 2.0
136 stars 102 forks source link

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 1 month ago

odeke-em commented 1 month ago

Throughout this package and in my journey of tracing this package, I've encountered the pattern of this form https://github.com/googleapis/nodejs-spanner/blob/2a19ef1af4f6fd1b81d08afc15db76007859a0b9/src/database.ts#L3608-L3628

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. https://github.com/googleapis/nodejs-spanner/blob/2a19ef1af4f6fd1b81d08afc15db76007859a0b9/src/database.ts#L2097-L2103

alkatrivedi commented 1 month ago

@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 commented 1 month ago

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

odeke-em commented 1 month ago

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

surbhigarg92 commented 2 weeks ago

@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 ?