Skip to content

Commit 2749751

Browse files
samikshya-dbsamikshya-db
committed
refactor: simplify TelemetryClient.close() and TelemetryClientProvider
- close() made synchronous (no I/O, no reason for async); uses finally to guarantee this.closed=true even when logger throws - TelemetryClientProvider.releaseClient() made synchronous for the same reason - Misleading ConcurrentHashMap reference removed from docstring - getRefCount/getActiveClients marked @internal (test-only surface) - Update tests to match: rejects→throws stubs, remove await on sync calls Co-authored-by: samikshya-chand_data
1 parent 990ec15 commit 2749751

4 files changed

Lines changed: 41 additions & 59 deletions

File tree

‎lib/telemetry/TelemetryClient.ts‎

Lines changed: 5 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,24 +48,16 @@ class TelemetryClient {
4848
* Closes the telemetry client and releases resources.
4949
* Should only be called by TelemetryClientProvider when reference count reaches zero.
5050
*/
51-
async close(): Promise<void> {
51+
close(): void {
5252
if (this.closed) {
5353
return;
5454
}
55-
5655
try {
57-
const logger = this.context.getLogger();
58-
logger.log(LogLevel.debug, `Closing TelemetryClient for host: ${this.host}`);
59-
this.closed = true;
60-
} catch (error: any) {
61-
// Swallow all exceptions per requirement
56+
this.context.getLogger().log(LogLevel.debug, `Closing TelemetryClient for host: ${this.host}`);
57+
} catch {
58+
// swallow
59+
} finally {
6260
this.closed = true;
63-
try {
64-
const logger = this.context.getLogger();
65-
logger.log(LogLevel.debug, `Error closing TelemetryClient: ${error.message}`);
66-
} catch (logError: any) {
67-
// If even logging fails, silently swallow
68-
}
6961
}
7062
}
7163
}

‎lib/telemetry/TelemetryClientProvider.ts‎

Lines changed: 7 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -32,8 +32,9 @@ interface TelemetryClientHolder {
3232
* Prevents rate limiting by sharing clients across connections to the same host.
3333
* Instance-based (not singleton), stored in DBSQLClient.
3434
*
35-
* Pattern from JDBC TelemetryClientFactory.java:27 with
36-
* ConcurrentHashMap<String, TelemetryClientHolder>.
35+
* Reference counts are incremented synchronously so there are no async races
36+
* on the count itself. The map entry is deleted before awaiting close() so a
37+
* concurrent getOrCreateClient call always gets a fresh instance.
3738
*/
3839
class TelemetryClientProvider {
3940
private clients: Map<string, TelemetryClientHolder>;
@@ -79,7 +80,7 @@ class TelemetryClientProvider {
7980
*
8081
* @param host The host identifier
8182
*/
82-
async releaseClient(host: string): Promise<void> {
83+
releaseClient(host: string): void {
8384
const logger = this.context.getLogger();
8485
const holder = this.clients.get(host);
8586

@@ -98,7 +99,7 @@ class TelemetryClientProvider {
9899
if (holder.refCount <= 0) {
99100
this.clients.delete(host);
100101
try {
101-
await holder.client.close();
102+
holder.client.close();
102103
logger.log(LogLevel.debug, `Closed and removed TelemetryClient for host: ${host}`);
103104
} catch (error: any) {
104105
// Swallow all exceptions per requirement
@@ -108,20 +109,15 @@ class TelemetryClientProvider {
108109
}
109110

110111
/**
111-
* Gets the current reference count for a host's client.
112-
* Useful for testing and diagnostics.
113-
*
114-
* @param host The host identifier
115-
* @returns The reference count, or 0 if no client exists
112+
* @internal Exposed for testing only.
116113
*/
117114
getRefCount(host: string): number {
118115
const holder = this.clients.get(host);
119116
return holder ? holder.refCount : 0;
120117
}
121118

122119
/**
123-
* Gets all active clients.
124-
* Useful for testing and diagnostics.
120+
* @internal Exposed for testing only.
125121
*/
126122
getActiveClients(): Map<string, TelemetryClient> {
127123
const result = new Map<string, TelemetryClient>();

‎tests/unit/telemetry/TelemetryClient.test.ts‎

Lines changed: 11 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ describe('TelemetryClient', () => {
6363
const context = new ClientContextStub();
6464
const client = new TelemetryClient(context, HOST);
6565

66-
await client.close();
66+
client.close();
6767

6868
expect(client.isClosed()).to.be.true;
6969
});
@@ -74,7 +74,7 @@ describe('TelemetryClient', () => {
7474
const context = new ClientContextStub();
7575
const client = new TelemetryClient(context, HOST);
7676

77-
await client.close();
77+
client.close();
7878

7979
expect(client.isClosed()).to.be.true;
8080
});
@@ -84,7 +84,7 @@ describe('TelemetryClient', () => {
8484
const logSpy = sinon.spy(context.logger, 'log');
8585
const client = new TelemetryClient(context, HOST);
8686

87-
await client.close();
87+
client.close();
8888

8989
expect(logSpy.calledWith(LogLevel.debug, `Closing TelemetryClient for host: ${HOST}`)).to.be.true;
9090
});
@@ -94,10 +94,10 @@ describe('TelemetryClient', () => {
9494
const logSpy = sinon.spy(context.logger, 'log');
9595
const client = new TelemetryClient(context, HOST);
9696

97-
await client.close();
97+
client.close();
9898
const firstCallCount = logSpy.callCount;
9999

100-
await client.close();
100+
client.close();
101101

102102
// Should not log again on second close
103103
expect(logSpy.callCount).to.equal(firstCallCount);
@@ -113,26 +113,20 @@ describe('TelemetryClient', () => {
113113
sinon.stub(context.logger, 'log').throws(error);
114114

115115
// Should not throw
116-
await client.close();
116+
client.close();
117117
// If we get here without throwing, the test passes
118118
expect(true).to.be.true;
119119
});
120120

121-
it('should log errors at debug level only', async () => {
121+
it('should still set closed when logger throws', () => {
122122
const context = new ClientContextStub();
123123
const client = new TelemetryClient(context, HOST);
124-
const error = new Error('Test error');
125124

126-
// Stub logger to throw on first call, succeed on second
127-
const logStub = sinon.stub(context.logger, 'log');
128-
logStub.onFirstCall().throws(error);
129-
logStub.onSecondCall().returns();
125+
sinon.stub(context.logger, 'log').throws(new Error('Logger error'));
130126

131-
await client.close();
127+
client.close();
132128

133-
// Second call should log the error at debug level
134-
expect(logStub.secondCall.args[0]).to.equal(LogLevel.debug);
135-
expect(logStub.secondCall.args[1]).to.include('Error closing TelemetryClient');
129+
expect(client.isClosed()).to.be.true;
136130
});
137131
});
138132

@@ -151,7 +145,7 @@ describe('TelemetryClient', () => {
151145
const logSpy = sinon.spy(context.logger, 'log');
152146
const client = new TelemetryClient(context, HOST);
153147

154-
await client.close();
148+
client.close();
155149

156150
logSpy.getCalls().forEach((call) => {
157151
expect(call.args[0]).to.equal(LogLevel.debug);

‎tests/unit/telemetry/TelemetryClientProvider.test.ts‎

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,10 @@ describe('TelemetryClientProvider', () => {
124124
provider.getOrCreateClient(HOST1);
125125
expect(provider.getRefCount(HOST1)).to.equal(3);
126126

127-
await provider.releaseClient(HOST1);
127+
provider.releaseClient(HOST1);
128128
expect(provider.getRefCount(HOST1)).to.equal(2);
129129

130-
await provider.releaseClient(HOST1);
130+
provider.releaseClient(HOST1);
131131
expect(provider.getRefCount(HOST1)).to.equal(1);
132132
});
133133

@@ -138,7 +138,7 @@ describe('TelemetryClientProvider', () => {
138138
const client = provider.getOrCreateClient(HOST1);
139139
const closeSpy = sinon.spy(client, 'close');
140140

141-
await provider.releaseClient(HOST1);
141+
provider.releaseClient(HOST1);
142142

143143
expect(closeSpy.calledOnce).to.be.true;
144144
expect(client.isClosed()).to.be.true;
@@ -151,7 +151,7 @@ describe('TelemetryClientProvider', () => {
151151
provider.getOrCreateClient(HOST1);
152152
expect(provider.getActiveClients().size).to.equal(1);
153153

154-
await provider.releaseClient(HOST1);
154+
provider.releaseClient(HOST1);
155155

156156
expect(provider.getActiveClients().size).to.equal(0);
157157
expect(provider.getRefCount(HOST1)).to.equal(0);
@@ -166,7 +166,7 @@ describe('TelemetryClientProvider', () => {
166166
provider.getOrCreateClient(HOST1);
167167
const closeSpy = sinon.spy(client, 'close');
168168

169-
await provider.releaseClient(HOST1);
169+
provider.releaseClient(HOST1);
170170

171171
expect(closeSpy.called).to.be.false;
172172
expect(client.isClosed()).to.be.false;
@@ -178,7 +178,7 @@ describe('TelemetryClientProvider', () => {
178178
const provider = new TelemetryClientProvider(context);
179179
const logSpy = sinon.spy(context.logger, 'log');
180180

181-
await provider.releaseClient(HOST1);
181+
provider.releaseClient(HOST1);
182182

183183
expect(logSpy.calledWith(LogLevel.debug, `No TelemetryClient found for host: ${HOST1}`)).to.be.true;
184184
});
@@ -191,7 +191,7 @@ describe('TelemetryClientProvider', () => {
191191
provider.getOrCreateClient(HOST1);
192192
provider.getOrCreateClient(HOST1);
193193

194-
await provider.releaseClient(HOST1);
194+
provider.releaseClient(HOST1);
195195

196196
expect(logSpy.calledWith(LogLevel.debug, `TelemetryClient reference count for ${HOST1}: 1`)).to.be.true;
197197
});
@@ -202,7 +202,7 @@ describe('TelemetryClientProvider', () => {
202202
const logSpy = sinon.spy(context.logger, 'log');
203203

204204
provider.getOrCreateClient(HOST1);
205-
await provider.releaseClient(HOST1);
205+
provider.releaseClient(HOST1);
206206

207207
expect(logSpy.calledWith(LogLevel.debug, `Closed and removed TelemetryClient for host: ${HOST1}`)).to.be.true;
208208
});
@@ -213,10 +213,10 @@ describe('TelemetryClientProvider', () => {
213213

214214
const client = provider.getOrCreateClient(HOST1);
215215
const error = new Error('Close error');
216-
sinon.stub(client, 'close').rejects(error);
216+
sinon.stub(client, 'close').throws(error);
217217
const logSpy = sinon.spy(context.logger, 'log');
218218

219-
await provider.releaseClient(HOST1);
219+
provider.releaseClient(HOST1);
220220

221221
expect(logSpy.calledWith(LogLevel.debug, `Error releasing TelemetryClient: ${error.message}`)).to.be.true;
222222
});
@@ -236,7 +236,7 @@ describe('TelemetryClientProvider', () => {
236236
expect(provider.getRefCount(HOST1)).to.equal(2);
237237
expect(provider.getRefCount(HOST2)).to.equal(3);
238238

239-
await provider.releaseClient(HOST1);
239+
provider.releaseClient(HOST1);
240240

241241
expect(provider.getRefCount(HOST1)).to.equal(1);
242242
expect(provider.getRefCount(HOST2)).to.equal(3);
@@ -250,15 +250,15 @@ describe('TelemetryClientProvider', () => {
250250
provider.getOrCreateClient(HOST1);
251251
const client2 = provider.getOrCreateClient(HOST2);
252252

253-
await provider.releaseClient(HOST1);
253+
provider.releaseClient(HOST1);
254254
expect(client1.isClosed()).to.be.false;
255255
expect(provider.getActiveClients().size).to.equal(2);
256256

257-
await provider.releaseClient(HOST1);
257+
provider.releaseClient(HOST1);
258258
expect(client1.isClosed()).to.be.true;
259259
expect(provider.getActiveClients().size).to.equal(1);
260260

261-
await provider.releaseClient(HOST2);
261+
provider.releaseClient(HOST2);
262262
expect(client2.isClosed()).to.be.true;
263263
expect(provider.getActiveClients().size).to.equal(0);
264264
});
@@ -284,7 +284,7 @@ describe('TelemetryClientProvider', () => {
284284
const client1 = provider.getOrCreateClient(HOST1);
285285
const client2 = provider.getOrCreateClient(HOST2);
286286

287-
await provider.releaseClient(HOST1);
287+
provider.releaseClient(HOST1);
288288

289289
expect(client1.isClosed()).to.be.true;
290290
expect(client2.isClosed()).to.be.false;
@@ -343,7 +343,7 @@ describe('TelemetryClientProvider', () => {
343343
provider.getOrCreateClient(HOST1);
344344
provider.getOrCreateClient(HOST2);
345345

346-
await provider.releaseClient(HOST1);
346+
provider.releaseClient(HOST1);
347347

348348
const clients = provider.getActiveClients();
349349

@@ -373,9 +373,9 @@ describe('TelemetryClientProvider', () => {
373373
const logSpy = sinon.spy(context.logger, 'log');
374374

375375
const client = provider.getOrCreateClient(HOST1);
376-
sinon.stub(client, 'close').rejects(new Error('Test error'));
376+
sinon.stub(client, 'close').throws(new Error('Test error'));
377377

378-
await provider.releaseClient(HOST1);
378+
provider.releaseClient(HOST1);
379379

380380
const errorLogs = logSpy.getCalls().filter((call) => call.args[1].includes('Error releasing'));
381381
expect(errorLogs.length).to.be.greaterThan(0);

0 commit comments

Comments
 (0)