Skip to content

Commit 4d4f57d

Browse files
eleanorjboydeleanorjboyd
committed
track all disposables
1 parent b313fc7 commit 4d4f57d

File tree

3 files changed

+36
-41
lines changed

3 files changed

+36
-41
lines changed

‎src/client/testing/testController/common/discoveryHelpers.ts‎

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -84,17 +84,10 @@ export function createProcessHandlers(
8484
const out = fixLogLinesNoTrailing(data.toString());
8585
traceError(out);
8686
},
87-
onExit: (code: number | null, signal: NodeJS.Signals | null) => {
87+
onExit: (code: number | null, _signal: NodeJS.Signals | null) => {
8888
// The 'exit' event fires when the process terminates, but streams may still be open.
89-
if (!isSuccessCode(code)) {
90-
const exitCodeNote =
91-
allowedSuccessCodes.length > 0
92-
? ` Note: Exit codes ${allowedSuccessCodes.join(', ')} are also treated as success.`
93-
: '';
94-
traceError(
95-
`${testProvider} discovery subprocess exited with code ${code} and signal ${signal} for workspace ${uri.fsPath}.${exitCodeNote}`,
96-
);
97-
} else if (code === 0) {
89+
// Only log verbose success message here; error handling happens in onClose.
90+
if (isSuccessCode(code)) {
9891
traceVerbose(`${testProvider} discovery subprocess exited successfully for workspace ${uri.fsPath}`);
9992
}
10093
},

‎src/client/testing/testController/pytest/pytestDiscoveryAdapter.ts‎

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33
import * as path from 'path';
4-
import { CancellationToken, Uri } from 'vscode';
4+
import { CancellationToken, Disposable, Uri } from 'vscode';
55
import { ChildProcess } from 'child_process';
66
import {
77
ExecutionFactoryCreateWithEnvironmentOptions,
@@ -64,6 +64,12 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
6464
// Setup process handlers deferred (used by both execution paths)
6565
const deferredTillExecClose: Deferred<void> = createTestingDeferred();
6666

67+
// Collect all disposables related to discovery to handle cleanup in finally block
68+
const disposables: Disposable[] = [];
69+
if (tokenDisposable) {
70+
disposables.push(tokenDisposable);
71+
}
72+
6773
try {
6874
// Build pytest command and arguments
6975
const settings = this.configSettings.getSettings(uri);
@@ -105,6 +111,9 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
105111
const envExtCancellationHandler = token?.onCancellationRequested(() => {
106112
cleanupOnCancellation('pytest', proc, deferredTillExecClose, discoveryPipeCancellation, uri);
107113
});
114+
if (envExtCancellationHandler) {
115+
disposables.push(envExtCancellationHandler);
116+
}
108117
proc.stdout.on('data', handlers.onStdout);
109118
proc.stderr.on('data', handlers.onStderr);
110119
proc.onExit((code, signal) => {
@@ -113,7 +122,6 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
113122
});
114123

115124
await deferredTillExecClose.promise;
116-
envExtCancellationHandler?.dispose();
117125
traceInfo(`Pytest discovery completed for workspace ${uri.fsPath}`);
118126
return;
119127
}
@@ -151,13 +159,15 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
151159
};
152160

153161
let resultProc: ChildProcess | undefined;
154-
let cancellationHandler: import('vscode').Disposable | undefined;
155162

156163
// Set up cancellation handler after all early return checks
157-
cancellationHandler = token?.onCancellationRequested(() => {
164+
const cancellationHandler = token?.onCancellationRequested(() => {
158165
traceInfo(`Cancellation requested during pytest discovery for workspace ${uri.fsPath}`);
159166
cleanupOnCancellation('pytest', resultProc, deferredTillExecClose, discoveryPipeCancellation, uri);
160167
});
168+
if (cancellationHandler) {
169+
disposables.push(cancellationHandler);
170+
}
161171

162172
try {
163173
const result = execService.execObservable(commandArgs, spawnOptions);
@@ -166,29 +176,19 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
166176
if (!resultProc) {
167177
traceError(`Failed to spawn pytest discovery subprocess for workspace ${uri.fsPath}`);
168178
deferredTillExecClose.resolve();
169-
cancellationHandler?.dispose();
170179
return;
171180
}
172181
traceInfo(`Started pytest discovery subprocess (execution factory) for workspace ${uri.fsPath}`);
173182
} catch (error) {
174183
traceError(`Error spawning pytest discovery subprocess for workspace ${uri.fsPath}: ${error}`);
175184
deferredTillExecClose.resolve();
176-
cancellationHandler?.dispose();
177185
throw error;
178186
}
179187
resultProc.stdout?.on('data', handlers.onStdout);
180188
resultProc.stderr?.on('data', handlers.onStderr);
181189
resultProc.on('exit', handlers.onExit);
182190
resultProc.on('close', handlers.onClose);
183191

184-
cancellationHandler?.dispose();
185-
186-
// Check for early cancellation before awaiting
187-
if (token?.isCancellationRequested) {
188-
traceInfo(`Pytest discovery was cancelled before process completion for workspace ${uri.fsPath}`);
189-
deferredTillExecClose.resolve();
190-
}
191-
192192
traceVerbose(`Waiting for pytest discovery subprocess to complete for workspace ${uri.fsPath}`);
193193
await deferredTillExecClose.promise;
194194
traceInfo(`Pytest discovery completed for workspace ${uri.fsPath}`);
@@ -197,8 +197,9 @@ export class PytestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
197197
deferredTillExecClose.resolve();
198198
throw error;
199199
} finally {
200-
traceVerbose(`Cleaning up pytest discovery resources for workspace ${uri.fsPath}`);
201-
tokenDisposable?.dispose();
200+
// Dispose all cancellation handlers and event subscriptions
201+
disposables.forEach((d) => d.dispose());
202+
// Dispose the discovery pipe cancellation token
202203
discoveryPipeCancellation.dispose();
203204
}
204205
}

‎src/client/testing/testController/unittest/testDiscoveryAdapter.ts‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4-
import { CancellationToken, Uri } from 'vscode';
4+
import { CancellationToken, Disposable, Uri } from 'vscode';
55
import { ChildProcess } from 'child_process';
66
import { IConfigurationService } from '../../../common/types';
77
import { EXTENSION_ROOT_DIR } from '../../../constants';
@@ -62,6 +62,11 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
6262
// Setup process handlers deferred (used by both execution paths)
6363
const deferredTillExecClose = createTestingDeferred();
6464

65+
// Collect all disposables for cleanup in finally block
66+
const disposables: Disposable[] = [];
67+
if (tokenDisposable) {
68+
disposables.push(tokenDisposable);
69+
}
6570
try {
6671
// Build unittest command and arguments
6772
const settings = this.configSettings.getSettings(uri);
@@ -100,6 +105,9 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
100105
const envExtCancellationHandler = token?.onCancellationRequested(() => {
101106
cleanupOnCancellation('unittest', proc, deferredTillExecClose, discoveryPipeCancellation, uri);
102107
});
108+
if (envExtCancellationHandler) {
109+
disposables.push(envExtCancellationHandler);
110+
}
103111
proc.stdout.on('data', handlers.onStdout);
104112
proc.stderr.on('data', handlers.onStderr);
105113
proc.onExit((code, signal) => {
@@ -108,7 +116,6 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
108116
});
109117

110118
await deferredTillExecClose.promise;
111-
envExtCancellationHandler?.dispose();
112119
traceInfo(`Unittest discovery completed for workspace ${uri.fsPath}`);
113120
return;
114121
}
@@ -146,13 +153,15 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
146153
};
147154

148155
let resultProc: ChildProcess | undefined;
149-
let cancellationHandler: import('vscode').Disposable | undefined;
150156

151157
// Set up cancellation handler after all early return checks
152-
cancellationHandler = token?.onCancellationRequested(() => {
158+
const cancellationHandler = token?.onCancellationRequested(() => {
153159
traceInfo(`Cancellation requested during unittest discovery for workspace ${uri.fsPath}`);
154160
cleanupOnCancellation('unittest', resultProc, deferredTillExecClose, discoveryPipeCancellation, uri);
155161
});
162+
if (cancellationHandler) {
163+
disposables.push(cancellationHandler);
164+
}
156165

157166
try {
158167
const result = execService.execObservable(execArgs, spawnOptions);
@@ -161,29 +170,19 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
161170
if (!resultProc) {
162171
traceError(`Failed to spawn unittest discovery subprocess for workspace ${uri.fsPath}`);
163172
deferredTillExecClose.resolve();
164-
cancellationHandler?.dispose();
165173
return;
166174
}
167175
traceInfo(`Started unittest discovery subprocess (execution factory) for workspace ${uri.fsPath}`);
168176
} catch (error) {
169177
traceError(`Error spawning unittest discovery subprocess for workspace ${uri.fsPath}: ${error}`);
170178
deferredTillExecClose.resolve();
171-
cancellationHandler?.dispose();
172179
throw error;
173180
}
174181
resultProc.stdout?.on('data', handlers.onStdout);
175182
resultProc.stderr?.on('data', handlers.onStderr);
176183
resultProc.on('exit', handlers.onExit);
177184
resultProc.on('close', handlers.onClose);
178185

179-
cancellationHandler?.dispose();
180-
181-
// Check for early cancellation before awaiting
182-
if (token?.isCancellationRequested) {
183-
traceInfo(`Unittest discovery was cancelled before process completion for workspace ${uri.fsPath}`);
184-
deferredTillExecClose.resolve();
185-
}
186-
187186
traceVerbose(`Waiting for unittest discovery subprocess to complete for workspace ${uri.fsPath}`);
188187
await deferredTillExecClose.promise;
189188
traceInfo(`Unittest discovery completed for workspace ${uri.fsPath}`);
@@ -193,7 +192,9 @@ export class UnittestTestDiscoveryAdapter implements ITestDiscoveryAdapter {
193192
throw error;
194193
} finally {
195194
traceVerbose(`Cleaning up unittest discovery resources for workspace ${uri.fsPath}`);
196-
tokenDisposable?.dispose();
195+
// Dispose all cancellation handlers and event subscriptions
196+
disposables.forEach((d) => d.dispose());
197+
// Dispose the discovery pipe cancellation token
197198
discoveryPipeCancellation.dispose();
198199
}
199200
}

0 commit comments

Comments
 (0)