Skip to content

Commit 8a05ead

Browse files
award999award999
authored
Properly cleanup task event emitters (#1593)
* Properly cleanup task even emitters - Were not displosing of event emitters - Were cleaning up read-only process data listeners early * Create a new process instance for each execution * Wait for process to start before feeding input
1 parent 88d88ac commit 8a05ead

File tree

9 files changed

+161
-45
lines changed

9 files changed

+161
-45
lines changed

‎src/tasks/SwiftExecution.ts‎

Lines changed: 36 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,22 +26,47 @@ export interface SwiftExecutionOptions extends vscode.ProcessExecutionOptions {
2626
* control over how the task and `swift` process is executed and allows
2727
* us to capture and process the output of the `swift` process
2828
*/
29-
export class SwiftExecution extends vscode.CustomExecution {
29+
export class SwiftExecution extends vscode.CustomExecution implements vscode.Disposable {
30+
private readonly writeEmitter: vscode.EventEmitter<string> = new vscode.EventEmitter<string>();
31+
private readonly closeEmitter: vscode.EventEmitter<number | void> = new vscode.EventEmitter<
32+
number | void
33+
>();
34+
private disposables: vscode.Disposable[] = [];
35+
3036
constructor(
3137
public readonly command: string,
3238
public readonly args: string[],
3339
public readonly options: SwiftExecutionOptions,
34-
private readonly swiftProcess: SwiftProcess = options.readOnlyTerminal
35-
? new ReadOnlySwiftProcess(command, args, options)
36-
: new SwiftPtyProcess(command, args, options)
40+
private swiftProcess: SwiftProcess | undefined = undefined
3741
) {
3842
super(async () => {
39-
return new SwiftPseudoterminal(swiftProcess, options.presentation || {});
43+
const createSwiftProcess = () => {
44+
if (!swiftProcess) {
45+
this.swiftProcess = options.readOnlyTerminal
46+
? new ReadOnlySwiftProcess(command, args, options)
47+
: new SwiftPtyProcess(command, args, options);
48+
this.listen(this.swiftProcess);
49+
}
50+
return this.swiftProcess!;
51+
};
52+
return new SwiftPseudoterminal(createSwiftProcess, options.presentation || {});
4053
});
54+
if (this.swiftProcess) {
55+
this.listen(this.swiftProcess);
56+
}
57+
}
58+
59+
private listen(swiftProcess: SwiftProcess) {
60+
this.dispose();
61+
this.disposables.push(
62+
swiftProcess.onDidWrite(e => this.writeEmitter.fire(e)),
63+
swiftProcess.onDidClose(e => this.closeEmitter.fire(e))
64+
);
65+
}
4166

42-
this.swiftProcess = swiftProcess;
43-
this.onDidWrite = swiftProcess.onDidWrite;
44-
this.onDidClose = swiftProcess.onDidClose;
67+
dispose() {
68+
this.disposables.forEach(d => d.dispose());
69+
this.disposables = [];
4570
}
4671

4772
/**
@@ -50,20 +75,20 @@ export class SwiftExecution extends vscode.CustomExecution {
5075
*
5176
* @see {@link SwiftProcess.onDidWrite}
5277
*/
53-
onDidWrite: vscode.Event<string>;
78+
onDidWrite: vscode.Event<string> = this.writeEmitter.event;
5479

5580
/**
5681
* Bubbles up the {@link SwiftProcess.onDidClose onDidClose} event
5782
* from the {@link SwiftProcess}
5883
*
5984
* @see {@link SwiftProcess.onDidClose}
6085
*/
61-
onDidClose: vscode.Event<number | void>;
86+
onDidClose: vscode.Event<number | void> = this.closeEmitter.event;
6287

6388
/**
6489
* Terminate the underlying executable.
6590
*/
6691
terminate(signal?: NodeJS.Signals) {
67-
this.swiftProcess.terminate(signal);
92+
this.swiftProcess?.terminate(signal);
6893
}
6994
}

‎src/tasks/SwiftProcess.ts‎

Lines changed: 40 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ import * as vscode from "vscode";
1919
import { requireNativeModule } from "../utilities/native";
2020
const { spawn } = requireNativeModule<typeof nodePty>("node-pty");
2121

22-
export interface SwiftProcess {
22+
export interface SwiftProcess extends vscode.Disposable {
2323
/**
2424
* Resolved path to the `swift` executable
2525
*/
@@ -73,7 +73,7 @@ export interface SwiftProcess {
7373
setDimensions(dimensions: vscode.TerminalDimensions): void;
7474
}
7575

76-
class CloseHandler {
76+
class CloseHandler implements vscode.Disposable {
7777
private readonly closeEmitter: vscode.EventEmitter<number | void> = new vscode.EventEmitter<
7878
number | void
7979
>();
@@ -94,6 +94,10 @@ class CloseHandler {
9494
}
9595
}
9696

97+
dispose() {
98+
this.closeEmitter.dispose();
99+
}
100+
97101
private queueClose() {
98102
this.closeTimeout = setTimeout(() => {
99103
this.closeEmitter.fire(this.exitCode);
@@ -110,14 +114,22 @@ export class SwiftPtyProcess implements SwiftProcess {
110114
private readonly writeEmitter: vscode.EventEmitter<string> = new vscode.EventEmitter<string>();
111115
private readonly errorEmitter: vscode.EventEmitter<Error> = new vscode.EventEmitter<Error>();
112116
private readonly closeHandler: CloseHandler = new CloseHandler();
117+
private disposables: vscode.Disposable[] = [];
113118

114119
private spawnedProcess?: nodePty.IPty;
115120

116121
constructor(
117122
public readonly command: string,
118123
public readonly args: string[],
119124
private options: vscode.ProcessExecutionOptions = {}
120-
) {}
125+
) {
126+
this.disposables.push(
127+
this.spawnEmitter,
128+
this.writeEmitter,
129+
this.errorEmitter,
130+
this.closeHandler
131+
);
132+
}
121133

122134
spawn(): void {
123135
try {
@@ -147,6 +159,11 @@ export class SwiftPtyProcess implements SwiftProcess {
147159
this.closeHandler.handle();
148160
}
149161
});
162+
this.disposables.push(
163+
this.onDidClose(() => {
164+
this.dispose();
165+
})
166+
);
150167
} catch (error) {
151168
this.errorEmitter.fire(new Error(`${error}`));
152169
this.closeHandler.handle();
@@ -173,6 +190,10 @@ export class SwiftPtyProcess implements SwiftProcess {
173190
this.spawnedProcess?.resize(dimensions.columns, dimensions.rows);
174191
}
175192

193+
dispose() {
194+
this.disposables.forEach(d => d.dispose());
195+
}
196+
176197
onDidSpawn: vscode.Event<void> = this.spawnEmitter.event;
177198

178199
onDidWrite: vscode.Event<string> = this.writeEmitter.event;
@@ -197,14 +218,22 @@ export class ReadOnlySwiftProcess implements SwiftProcess {
197218
private readonly writeEmitter: vscode.EventEmitter<string> = new vscode.EventEmitter<string>();
198219
private readonly errorEmitter: vscode.EventEmitter<Error> = new vscode.EventEmitter<Error>();
199220
private readonly closeHandler: CloseHandler = new CloseHandler();
221+
private disposables: vscode.Disposable[] = [];
200222

201223
private spawnedProcess: child_process.ChildProcessWithoutNullStreams | undefined;
202224

203225
constructor(
204226
public readonly command: string,
205227
public readonly args: string[],
206228
private readonly options: vscode.ProcessExecutionOptions = {}
207-
) {}
229+
) {
230+
this.disposables.push(
231+
this.spawnEmitter,
232+
this.writeEmitter,
233+
this.errorEmitter,
234+
this.closeHandler
235+
);
236+
}
208237

209238
spawn(): void {
210239
try {
@@ -231,12 +260,16 @@ export class ReadOnlySwiftProcess implements SwiftProcess {
231260

232261
this.spawnedProcess.once("exit", code => {
233262
this.closeHandler.handle(code ?? undefined);
234-
this.dispose();
235263
});
264+
265+
this.disposables.push(
266+
this.onDidClose(() => {
267+
this.dispose();
268+
})
269+
);
236270
} catch (error) {
237271
this.errorEmitter.fire(new Error(`${error}`));
238272
this.closeHandler.handle();
239-
this.dispose();
240273
}
241274
}
242275

@@ -260,6 +293,7 @@ export class ReadOnlySwiftProcess implements SwiftProcess {
260293
this.spawnedProcess?.stdout.removeAllListeners();
261294
this.spawnedProcess?.stderr.removeAllListeners();
262295
this.spawnedProcess?.removeAllListeners();
296+
this.disposables.forEach(d => d.dispose());
263297
}
264298

265299
onDidSpawn: vscode.Event<void> = this.spawnEmitter.event;

‎src/tasks/SwiftPseudoterminal.ts‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -24,26 +24,25 @@ export class SwiftPseudoterminal implements vscode.Pseudoterminal, vscode.Dispos
2424
private readonly closeEmitter: vscode.EventEmitter<number | void> = new vscode.EventEmitter<
2525
number | void
2626
>();
27+
private swiftProcess: SwiftProcess | undefined;
2728

2829
constructor(
29-
private swiftProcess: SwiftProcess,
30+
private createSwiftProcess: () => SwiftProcess,
3031
private options: vscode.TaskPresentationOptions
3132
) {}
3233

3334
private disposables: vscode.Disposable[] = [];
3435

35-
get commandLine(): string {
36-
return [this.swiftProcess.command, ...this.swiftProcess.args].join(" ");
37-
}
38-
3936
open(initialDimensions: vscode.TerminalDimensions | undefined): void {
37+
this.swiftProcess = this.createSwiftProcess();
38+
const commandLine = [this.swiftProcess.command, ...this.swiftProcess.args].join(" ");
4039
try {
4140
// Convert the pty's events to the ones expected by the Tasks API
4241
this.disposables.push(
4342
this.swiftProcess.onDidSpawn(() => {
4443
// Display the actual command line that we're executing. `echo` defaults to true.
4544
if (this.options.echo !== false) {
46-
this.writeEmitter.fire(`> ${this.commandLine}\n\n\r`);
45+
this.writeEmitter.fire(`> ${commandLine}\n\n\r`);
4746
}
4847
}),
4948
this.swiftProcess.onDidWrite(data => {
@@ -52,7 +51,7 @@ export class SwiftPseudoterminal implements vscode.Pseudoterminal, vscode.Dispos
5251
}),
5352
this.swiftProcess.onDidThrowError(e => {
5453
void vscode.window.showErrorMessage(
55-
`Failed to run Swift command "${this.commandLine}":\n${e}`
54+
`Failed to run Swift command "${commandLine}":\n${e}`
5655
);
5756
this.closeEmitter.fire();
5857
this.dispose();
@@ -90,21 +89,24 @@ export class SwiftPseudoterminal implements vscode.Pseudoterminal, vscode.Dispos
9089
const buf: Buffer = Buffer.from(data);
9190
// Terminate process on ctrl+c
9291
if (buf.length === 1 && buf[0] === 3) {
93-
this.swiftProcess.terminate();
92+
this.swiftProcess?.terminate();
9493
} else {
95-
this.swiftProcess.handleInput(data);
94+
this.swiftProcess?.handleInput(data);
9695
}
9796
}
9897

9998
setDimensions(dimensions: vscode.TerminalDimensions): void {
100-
this.swiftProcess.setDimensions(dimensions);
99+
this.swiftProcess?.setDimensions(dimensions);
101100
}
102101

103102
onDidWrite: vscode.Event<string> = this.writeEmitter.event;
104103

105104
onDidClose: vscode.Event<number | void> = this.closeEmitter.event;
106105

107106
close(): void {
108-
this.swiftProcess.terminate();
107+
this.swiftProcess?.terminate();
108+
// Terminal may be re-used so only dispose of these on close
109+
this.writeEmitter.dispose();
110+
this.closeEmitter.dispose();
109111
}
110112
}

‎test/fixtures.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,12 +70,19 @@ export class TestSwiftProcess implements SwiftProcess {
7070
close(exitCode: number): void {
7171
this.exitCode = exitCode;
7272
this.closeEmitter.fire(exitCode);
73+
this.dispose();
7374
}
7475

7576
terminate(): void {
7677
this.close(8);
7778
}
7879

80+
dispose() {
81+
[this.spawnEmitter, this.writeEmitter, this.errorEmitter, this.closeEmitter].forEach(d =>
82+
d.dispose()
83+
);
84+
}
85+
7986
// These instantaneous task processes can lead to some
8087
// events being missed while running under test
8188

‎test/integration-tests/DiagnosticsManager.test.ts‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,11 @@
1515
import * as assert from "assert";
1616
import * as vscode from "vscode";
1717
import { SwiftToolchain } from "../../src/toolchain/toolchain";
18-
import { executeTaskAndWaitForResult, waitForNoRunningTasks } from "../utilities/tasks";
18+
import {
19+
executeTaskAndWaitForResult,
20+
waitForNoRunningTasks,
21+
waitForStartTaskProcess,
22+
} from "../utilities/tasks";
1923
import { WorkspaceContext } from "../../src/WorkspaceContext";
2024
import { testAssetUri, testSwiftTask } from "../fixtures";
2125
import { createBuildAllTask, resetBuildAllTaskCache } from "../../src/tasks/SwiftTaskProvider";
@@ -438,7 +442,9 @@ suite("DiagnosticsManager Test Suite", function () {
438442

439443
test("Parse partial line", async () => {
440444
const fixture = testSwiftTask("swift", ["build"], workspaceFolder, toolchain);
445+
const startPromise = waitForStartTaskProcess(fixture.task);
441446
await vscode.tasks.executeTask(fixture.task);
447+
await startPromise;
442448
// Wait to spawn before writing
443449
fixture.process.write(`${mainUri.fsPath}:13:5: err`, "");
444450
fixture.process.write("or: Cannot find 'fo", "");
@@ -452,7 +458,9 @@ suite("DiagnosticsManager Test Suite", function () {
452458
// https://github.com/apple/swift/issues/73973
453459
test("Ignore duplicates", async () => {
454460
const fixture = testSwiftTask("swift", ["build"], workspaceFolder, toolchain);
461+
const startPromise = waitForStartTaskProcess(fixture.task);
455462
await vscode.tasks.executeTask(fixture.task);
463+
await startPromise;
456464
// Wait to spawn before writing
457465
const output = `${mainUri.fsPath}:13:5: error: Cannot find 'foo' in scope`;
458466
fixture.process.write(output);
@@ -468,7 +476,9 @@ suite("DiagnosticsManager Test Suite", function () {
468476

469477
test("New set of swiftc diagnostics clear old list", async () => {
470478
let fixture = testSwiftTask("swift", ["build"], workspaceFolder, toolchain);
479+
let startPromise = waitForStartTaskProcess(fixture.task);
471480
await vscode.tasks.executeTask(fixture.task);
481+
await startPromise;
472482
// Wait to spawn before writing
473483
fixture.process.write(`${mainUri.fsPath}:13:5: error: Cannot find 'foo' in scope`);
474484
fixture.process.close(1);
@@ -480,7 +490,9 @@ suite("DiagnosticsManager Test Suite", function () {
480490

481491
// Run again but no diagnostics returned
482492
fixture = testSwiftTask("swift", ["build"], workspaceFolder, toolchain);
493+
startPromise = waitForStartTaskProcess(fixture.task);
483494
await vscode.tasks.executeTask(fixture.task);
495+
await startPromise;
484496
fixture.process.close(0);
485497
await waitForNoRunningTasks();
486498
diagnostics = vscode.languages.getDiagnostics(mainUri);

‎test/integration-tests/commands/dependency.test.ts‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,14 @@ suite("Dependency Commmands Test Suite", function () {
3535
async setup(ctx) {
3636
workspaceContext = ctx;
3737
depsContext = await folderInRootWorkspace("dependencies", workspaceContext);
38-
await waitForNoRunningTasks();
39-
await workspaceContext.focusFolder(depsContext);
4038
},
4139
});
4240

41+
setup(async () => {
42+
await workspaceContext.focusFolder(depsContext);
43+
await waitForNoRunningTasks();
44+
});
45+
4346
test("Swift: Update Package Dependencies", async function () {
4447
const result = await vscode.commands.executeCommand(Commands.UPDATE_DEPENDENCIES);
4548
expect(result).to.be.true;
@@ -50,7 +53,8 @@ suite("Dependency Commmands Test Suite", function () {
5053
expect(result).to.be.true;
5154
});
5255

53-
suite("Swift: Use Local Dependency", function () {
56+
// Skipping: https://github.com/swiftlang/vscode-swift/issues/1316
57+
suite.skip("Swift: Use Local Dependency", function () {
5458
let treeProvider: ProjectPanelProvider;
5559

5660
setup(async () => {
@@ -103,7 +107,6 @@ suite("Dependency Commmands Test Suite", function () {
103107
}
104108

105109
test("Swift: Reset Package Dependencies", async function () {
106-
this.skip(); // https://github.com/swiftlang/vscode-swift/issues/1316
107110
// spm reset after using local dependency is broken on windows
108111
if (process.platform === "win32") {
109112
this.skip();
@@ -120,7 +123,6 @@ suite("Dependency Commmands Test Suite", function () {
120123
});
121124

122125
test("Swift: Revert To Original Version", async function () {
123-
this.skip(); // https://github.com/swiftlang/vscode-swift/issues/1316
124126
await useLocalDependencyTest();
125127

126128
const result = await vscode.commands.executeCommand(

0 commit comments

Comments
 (0)