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
Conversation
…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.
hessjcg
changed the title
hessjcg
force-pushed
the
gh-421-refactor-refresh
branch
from
8743264 to
51dfbe4
Compare
| // The Connector class is the main public API to interact | ||
| // with the Cloud SQL Node.js Connector. | ||
| export class Connector { | ||
| private closed = false; |
There was a problem hiding this comment.
is closed really needed at the Connector level? Instance makes sense but what is the benefit for Connector level...
There was a problem hiding this comment.
This isn't necessary. I removed it.
| // 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> { |
There was a problem hiding this comment.
I don't fully understand this... how is this different then awaiting the promise of forceRefresh?
There was a problem hiding this comment.
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.
hessjcg
force-pushed
the
gh-421-refactor-refresh
branch
from
51dfbe4 to
75fdb37
Compare
| // The Connector class is the main public API to interact | ||
| // with the Cloud SQL Node.js Connector. | ||
| export class Connector { | ||
| private closed = false; |
There was a problem hiding this comment.
This isn't necessary. I removed it.
| }); | ||
| tlsSocket.once('error', async () => { | ||
| await cloudSqlInstance.forceRefresh(); | ||
| tlsSocket.once('error', () => { |
There was a problem hiding this comment.
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.
| // 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> { |
There was a problem hiding this comment.
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.
hessjcg
force-pushed
the
gh-421-refactor-refresh
branch
2 times, most recently
from
54ef048 to
d838204
Compare
| this.scheduleRefresh(0); | ||
|
|
||
| return new Promise(resolve => { | ||
| // setImmediate() to yield execution to allow other refresh background |
There was a problem hiding this comment.
this comment references setImmediate but we are using setTimeout...
There was a problem hiding this comment.
Fixed. setTimeout() is the correct function.
| // performRefresh() could be delayed by the rate limiter. So | ||
| // this instance may have been closed. |
There was a problem hiding this comment.
Can we update this wording a bit. "So this instance may have been closed" is a bit ambiguous...
| // starts a new refresh cycle but do not await on it | ||
| instance.close(); | ||
| await instance.forceRefresh(); | ||
| t.equal(metadataCount, 1, 'No refresh after close'); |
There was a problem hiding this comment.
does this mean we do not error on a closed instance?
There was a problem hiding this comment.
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
force-pushed
the
gh-421-refactor-refresh
branch
from
d838204 to
330c007
Compare
hessjcg
force-pushed
the
gh-421-refactor-refresh
branch
from
330c007 to
f4f69c1
Compare
|
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. |
2 participants
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.