Copilot commented on code in PR #462:
URL:
https://github.com/apache/cassandra-nodejs-driver/pull/462#discussion_r3270662892
##########
lib/control-connection.js:
##########
@@ -446,13 +441,31 @@ class ControlConnection extends events.EventEmitter {
}
/**
- * Acquires a new connection and refreshes topology and keyspace metadata.
+ * Acquires a new connection and refreshes topology and keyspace metadata,
with protection against concurrent refreshes.
* <p>When it fails obtaining a connection and there aren't any more hosts,
it schedules reconnection.</p>
* <p>When it fails obtaining the metadata, it marks connection and/or host
unusable and retries using the same
* iterator from query plan / host list</p>
* @param {Iterator<Host>} [hostIterator]
*/
async _refresh(hostIterator) {
+ if (this._refreshInProgress) {
+ return;
+ }
+ this._refreshInProgress = true;
+
+ try {
+ return await this._unsafeDoRefresh(hostIterator)
+ } finally {
+ this._refreshInProgress = false;
+ }
Review Comment:
When a refresh is already in progress, this returns immediately (resolved
promise) without waiting for the ongoing refresh. That can cause callers to
observe `cc.host` / `cc.connection` as `null` right after awaiting
`_refresh()`, because `_unsafeDoRefresh()` clears them early. Consider storing
the in-flight refresh promise and returning/awaiting it (or scheduling a
follow-up refresh) instead of short-circuiting.
##########
lib/control-connection.js:
##########
@@ -446,13 +441,31 @@ class ControlConnection extends events.EventEmitter {
}
/**
- * Acquires a new connection and refreshes topology and keyspace metadata.
+ * Acquires a new connection and refreshes topology and keyspace metadata,
with protection against concurrent refreshes.
* <p>When it fails obtaining a connection and there aren't any more hosts,
it schedules reconnection.</p>
* <p>When it fails obtaining the metadata, it marks connection and/or host
unusable and retries using the same
* iterator from query plan / host list</p>
* @param {Iterator<Host>} [hostIterator]
*/
async _refresh(hostIterator) {
+ if (this._refreshInProgress) {
+ return;
+ }
+ this._refreshInProgress = true;
+
+ try {
+ return await this._unsafeDoRefresh(hostIterator)
Review Comment:
Missing semicolon after `return await this._unsafeDoRefresh(hostIterator)`;
this file consistently uses semicolons and this is likely to fail linting.
##########
test/integration/short/control-connection-tests.js:
##########
@@ -153,6 +153,25 @@ describe('ControlConnection', function () {
assert.strictEqual(cc.hosts.length, 1);
});
+ it('should not break when refreshing concurrently', async () => {
+ const cc = newInstance();
+ cc.options.policies.loadBalancing = new
policies.loadBalancing.RoundRobinPolicy();
+ disposeAfter(cc);
+
+ await cc.init();
+ await new Promise(r => cc.options.policies.loadBalancing.init(null,
cc.hosts, r));
+
+ const refreshPromises = [];
+ // randomly emit cc._refresh 100 times
+ for (let i = 0; i < 100; i++) {
+ refreshPromises.push(cc._refresh());
+ await helper.delayAsync(~~(Math.random() * 100));
+ }
+ await Promise.all(refreshPromises);
Review Comment:
This test is nondeterministic and can be flaky/slow: it uses `Math.random()`
and serial `delayAsync()` up to 100ms over 100 iterations (multi-second
average). Consider making it deterministic (e.g., fire a fixed batch of
concurrent `_refresh()` calls without random delays, or use a predictable delay
pattern) and explicitly waiting for the in-flight refresh to complete before
asserting on `cc.host`/`cc.connection`.
##########
lib/promise-utils.js:
##########
@@ -149,7 +149,7 @@ function times(count, limit, fn) {
* @returns {undefined}
*/
function toBackground(promise) {
- promise.catch(() => {});
+ promise?.catch(() => {});
}
Review Comment:
`toBackground()` now accepts `undefined`/`null` (due to optional chaining),
but the JSDoc still declares `@param {Promise} promise`. Update the type to
reflect the new behavior (e.g., `{Promise|undefined|null}`) to keep
documentation accurate.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
