refactor: Improve the refresh to allow connectors to close, part of #421. #424

hessjcg · 2025-03-05T16:40:57Z

Add CloudSqlInstance.close() to stop the refresh cycle permanently.

Change CloudSqlInstance.forceRefresh() so that it schedules a refresh and returns immediately.
This should not return a promise. Update test cases to account for changes.

…nd return immediately. This avoids the situation where a refresh process is forced to start but may never finish, causing the node process to hang indefiniately awaiting the refresh promise that does not resolve. This also adds a new method used in the test cases: CloudSqlInstance.refreshComplete() which returns a promise that will resolve when the refresh operation has finished.

.github/workflows/coverage.yml

jackwotherspoon · 2025-03-06T17:38:10Z

src/connector.ts

 // The Connector class is the main public API to interact
 // with the Cloud SQL Node.js Connector.
 export class Connector {
+  private closed = false;


is closed really needed at the Connector level? Instance makes sense but what is the benefit for Connector level...

This isn't necessary. I removed it.

jackwotherspoon · 2025-03-06T17:44:12Z

src/cloud-sql-instance.ts

+  // refreshComplete Returns a promise that resolves when the current refresh
+  // cycle has completed, either with success or failure. If no refresh is
+  // in progress, the promise will resolve immediately.
+  refreshComplete(): Promise<void> {


I don't fully understand this... how is this different then awaiting the promise of forceRefresh?

Good point. I've updated forceRefresh() to include all the logic.

Basically, forceRefresh() only needs to return a promise for testing. forceRefresh is used as fire-and-forget in the main connector code.

In tests, the promise should resolve regardless of whether it succeeds or fails. Otherwise, it could cause tests cases to hang where refresh fails.

.github/workflows/coverage.yml

hessjcg · 2025-03-10T16:32:02Z

src/connector.ts

 // The Connector class is the main public API to interact
 // with the Cloud SQL Node.js Connector.
 export class Connector {
+  private closed = false;


This isn't necessary. I removed it.

hessjcg · 2025-03-10T16:37:47Z

src/connector.ts

          });
-          tlsSocket.once('error', async () => {
-            await cloudSqlInstance.forceRefresh();
+          tlsSocket.once('error', () => {


This should not be async. Otherwise it will block the exit of the connector until the refresh is successful. This causes some failure test cases to hang and never exit.

hessjcg · 2025-03-10T16:48:36Z

src/cloud-sql-instance.ts

+  // refreshComplete Returns a promise that resolves when the current refresh
+  // cycle has completed, either with success or failure. If no refresh is
+  // in progress, the promise will resolve immediately.
+  refreshComplete(): Promise<void> {


Good point. I've updated forceRefresh() to include all the logic.

Basically, forceRefresh() only needs to return a promise for testing. forceRefresh is used as fire-and-forget in the main connector code.

In tests, the promise should resolve regardless of whether it succeeds or fails. Otherwise, it could cause tests cases to hang where refresh fails.

.github/workflows/coverage.yml

jackwotherspoon · 2025-03-10T17:48:06Z

src/cloud-sql-instance.ts

+    this.scheduleRefresh(0);
+
+    return new Promise(resolve => {
+      // setImmediate() to yield execution to allow other refresh background


this comment references setImmediate but we are using setTimeout...

Fixed. setTimeout() is the correct function.

jackwotherspoon · 2025-03-10T17:50:35Z

src/cloud-sql-instance.ts

+      // performRefresh() could be delayed by the rate limiter. So
+      // this instance may have been closed.


Can we update this wording a bit. "So this instance may have been closed" is a bit ambiguous...

jackwotherspoon · 2025-03-10T17:53:28Z

test/cloud-sql-instance.ts

+      // starts a new refresh cycle but do not await on it
+      instance.close();
+      await instance.forceRefresh();
+      t.equal(metadataCount, 1, 'No refresh after close');


does this mean we do not error on a closed instance?

We don't throw an error from forceRefresh(). The node connector will throw an error when trying to call connect() on a closed instance, but that's coming in a later PR.

hessjcg · 2025-03-10T20:52:37Z

Code Coverage / coverage decreased from 97% to 96% due to code paths in src/cloud-sql-instance.ts that are called after the CloudSQLInstance is closed. These code paths are impractical to test.

jackwotherspoon

LGTM ✅

hessjcg requested a review from a team as a code owner March 5, 2025 16:40

blunderbuss-gcf bot assigned jackwotherspoon Mar 5, 2025

hessjcg changed the title ~~refactor: Improve the refresh to allow connectors to close.~~ refactor: Improve the refresh to allow connectors to close, part of #421. Mar 5, 2025

hessjcg force-pushed the gh-421-refactor-refresh branch from 8743264 to 51dfbe4 Compare March 5, 2025 17:21

jackwotherspoon requested changes Mar 6, 2025

View reviewed changes

hessjcg requested a review from kgala2 March 7, 2025 15:50

hessjcg force-pushed the gh-421-refactor-refresh branch from 51dfbe4 to 75fdb37 Compare March 10, 2025 16:49

hessjcg commented Mar 10, 2025

View reviewed changes

hessjcg requested a review from jackwotherspoon March 10, 2025 16:50

hessjcg force-pushed the gh-421-refactor-refresh branch 2 times, most recently from 54ef048 to d838204 Compare March 10, 2025 16:55

jackwotherspoon requested changes Mar 10, 2025

View reviewed changes

hessjcg force-pushed the gh-421-refactor-refresh branch from d838204 to 330c007 Compare March 10, 2025 18:07

refactor: Add CloudSqlInstance.close() to stop the refresh cycle.

f4f69c1

hessjcg force-pushed the gh-421-refactor-refresh branch from 330c007 to f4f69c1 Compare March 10, 2025 18:13

jackwotherspoon approved these changes Mar 11, 2025

View reviewed changes

hessjcg merged commit 0e4853a into main Mar 11, 2025
25 of 26 checks passed

hessjcg deleted the gh-421-refactor-refresh branch March 11, 2025 16:58

		// performRefresh() could be delayed by the rate limiter. So
		// this instance may have been closed.

refactor: Improve the refresh to allow connectors to close, part of #421. #424

refactor: Improve the refresh to allow connectors to close, part of #421. #424

Uh oh!

Conversation

hessjcg commented Mar 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hessjcg commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jackwotherspoon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

hessjcg commented Mar 10, 2025 •

edited

Loading