fix(spanner): missing callback parameter in close#8127
fix(spanner): missing callback parameter in close#8127alkatrivedi wants to merge 2 commits intomainfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Spanner.close() method to support an optional callback and adds unit tests to verify the cleanup of cached clients. A review comment suggests improving the error handling in the close method to ensure that errors are still logged to the console if no callback is provided, preventing silent failures during cleanup.
|
/gemini |
|
|
||
| /** Closes this Spanner client and cleans up all resources used by it. */ | ||
| close(): void { | ||
| close(callback?: (err: Error | null) => void): void { |
There was a problem hiding this comment.
I think we should return promise as well for customers who would want to await rather than using callback ?
There was a problem hiding this comment.
the methods is included in promisifyAll here , hence, we need not to explicitly return the promise here
There was a problem hiding this comment.
since promisifyAll will promisify the method only during runtime, hence it the typescript customers wont be able to await on it
to fix the gap, I have added promise return type as well
| }); | ||
|
|
||
| describe('close', () => { | ||
| it('should close all cached clients', done => { |
There was a problem hiding this comment.
We should use async/await for tests instead of "done".
With "done" tests get stuck when it fails.
| // Wait for all clients to close, then do cleanup | ||
| Promise.all(promises) | ||
| .then(() => cleanup()) | ||
| .then( |
There was a problem hiding this comment.
This needs to be conditional. If async/await is used instead of callback then we need to return the promise.
2 participants
fixes: #8106