Skip to content

Commit 1c643ad

Browse files
ElianHughElianHugh
authored
(Refactoring) Simplify RMD child process disposal (#773)
- Contributes a helper function for creating disposables from objects - Simplifies disposal of child processes - Do not print lines to output when the process has already been terminated - use childProcess.spawn instead of childProcess.exec, to allow for faster process termination
1 parent 9f03d55 commit 1c643ad

4 files changed

Lines changed: 97 additions & 53 deletions

File tree

‎src/rmarkdown/knit.ts‎

Lines changed: 16 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ import * as fs from 'fs-extra';
44
import path = require('path');
55
import yaml = require('js-yaml');
66

7-
import { RMarkdownManager, KnitWorkingDirectory } from './manager';
7+
import { RMarkdownManager, KnitWorkingDirectory, DisposableProcess } from './manager';
88
import { runTextInTerm } from '../rTerminal';
99
import { rmdPreviewManager } from '../extension';
1010

@@ -26,23 +26,21 @@ interface IYamlFrontmatter {
2626
}
2727

2828
export class RMarkdownKnitManager extends RMarkdownManager {
29-
private async renderDocument(rPath: string, docPath: string, docName: string, yamlParams: IYamlFrontmatter, outputFormat?: string) {
29+
private async renderDocument(rDocumentPath: string, docPath: string, docName: string, yamlParams: IYamlFrontmatter, outputFormat?: string): Promise<DisposableProcess> {
3030
const openOutfile: boolean = util.config().get<boolean>('rmarkdown.knit.openOutputFile') ?? false;
3131
const knitWorkingDir = this.getKnitDir(knitDir, docPath);
3232
const knitWorkingDirText = knitWorkingDir ? `'${knitWorkingDir}'` : `NULL`;
33-
const knitCommand = await this.getKnitCommand(yamlParams, rPath, outputFormat);
34-
this.rPath = await util.getRpath(true);
33+
const knitCommand = await this.getKnitCommand(yamlParams, rDocumentPath, outputFormat);
34+
this.rPath = await util.getRpath();
3535

3636
const lim = '---vsc---';
3737
const re = new RegExp(`.*${lim}(.*)${lim}.*`, 'gms');
38-
const cmd = (
39-
`${this.rPath} --silent --slave --no-save --no-restore ` +
40-
`-e "knitr::opts_knit[['set']](root.dir = ${knitWorkingDirText})" ` +
41-
`-e "cat('${lim}', ` +
38+
const cmd =
39+
`knitr::opts_knit[['set']](root.dir = ${knitWorkingDirText});` +
40+
`cat('${lim}', ` +
4241
`${knitCommand}, ` +
4342
`'${lim}',` +
44-
`sep='')"`
45-
);
43+
`sep='')`;
4644

4745
const callback = (dat: string) => {
4846
const outputUrl = re.exec(dat)?.[0]?.replace(re, '$1');
@@ -68,7 +66,7 @@ export class RMarkdownKnitManager extends RMarkdownManager {
6866
return await this.knitWithProgress(
6967
{
7068
fileName: docName,
71-
filePath: rPath,
69+
filePath: rDocumentPath,
7270
cmd: cmd,
7371
rCmd: knitCommand,
7472
rOutputFormat: outputFormat,
@@ -122,7 +120,7 @@ export class RMarkdownKnitManager extends RMarkdownManager {
122120
`rmarkdown::render(${docPath})`;
123121
}
124122

125-
return knitCommand.replace(/['"]/g, '\\"');
123+
return knitCommand.replace(/['"]/g, '\'');
126124
}
127125

128126
// check if the workspace of the document is a R Markdown site.
@@ -217,12 +215,12 @@ export class RMarkdownKnitManager extends RMarkdownManager {
217215

218216
const isSaved = await util.saveDocument(wad);
219217
if (isSaved) {
220-
let rPath = util.ToRStringLiteral(wad.fileName, '"');
218+
let rDocumentPath = util.ToRStringLiteral(wad.fileName, '"');
221219
let encodingParam = util.config().get<string>('source.encoding');
222220
encodingParam = `encoding = "${encodingParam}"`;
223-
rPath = [rPath, encodingParam].join(', ');
221+
rDocumentPath = [rDocumentPath, encodingParam].join(', ');
224222
if (echo) {
225-
rPath = [rPath, 'echo = TRUE'].join(', ');
223+
rDocumentPath = [rDocumentPath, 'echo = TRUE'].join(', ');
226224
}
227225

228226
// allow users to opt out of background process
@@ -233,7 +231,7 @@ export class RMarkdownKnitManager extends RMarkdownManager {
233231
} else {
234232
this.busyUriStore.add(busyPath);
235233
await this.renderDocument(
236-
rPath,
234+
rDocumentPath,
237235
wad.uri.fsPath,
238236
path.basename(wad.uri.fsPath),
239237
this.getYamlFrontmatter(wad.uri.fsPath),
@@ -243,9 +241,9 @@ export class RMarkdownKnitManager extends RMarkdownManager {
243241
}
244242
} else {
245243
if (outputFormat === undefined) {
246-
void runTextInTerm(`rmarkdown::render(${rPath})`);
244+
void runTextInTerm(`rmarkdown::render(${rDocumentPath})`);
247245
} else {
248-
void runTextInTerm(`rmarkdown::render(${rPath}, "${outputFormat}")`);
246+
void runTextInTerm(`rmarkdown::render(${rDocumentPath}, '${outputFormat}')`);
249247
}
250248
}
251249
}

‎src/rmarkdown/manager.ts‎

Lines changed: 52 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,15 @@ export enum KnitWorkingDirectory {
88
workspaceRoot = 'workspace root',
99
}
1010

11+
export type DisposableProcess = cp.ChildProcessWithoutNullStreams & vscode.Disposable;
12+
13+
export interface IKnitRejection {
14+
cp: DisposableProcess;
15+
wasCancelled: boolean;
16+
}
17+
18+
const rMarkdownOutput: vscode.OutputChannel = vscode.window.createOutputChannel('R Markdown');
19+
1120
interface IKnitArgs {
1221
filePath: string;
1322
fileName: string;
@@ -18,13 +27,6 @@ interface IKnitArgs {
1827
onRejection?: (...args: unknown[]) => unknown;
1928
}
2029

21-
export interface IKnitRejection {
22-
cp: cp.ChildProcessWithoutNullStreams;
23-
wasCancelled: boolean;
24-
}
25-
26-
const rMarkdownOutput: vscode.OutputChannel = vscode.window.createOutputChannel('R Markdown');
27-
2830
export abstract class RMarkdownManager {
2931
protected rPath: string = undefined;
3032
protected rMarkdownOutput: vscode.OutputChannel = rMarkdownOutput;
@@ -51,18 +53,44 @@ export abstract class RMarkdownManager {
5153
}
5254
}
5355

54-
protected async knitDocument(args: IKnitArgs, token?: vscode.CancellationToken, progress?: vscode.Progress<unknown>): Promise<cp.ChildProcessWithoutNullStreams | IKnitRejection> {
56+
protected async knitDocument(args: IKnitArgs, token?: vscode.CancellationToken, progress?: vscode.Progress<unknown>): Promise<DisposableProcess | IKnitRejection> {
5557
// vscode.Progress auto-increments progress, so we use this
5658
// variable to set progress to a specific number
5759
let currentProgress = 0;
58-
return await new Promise<cp.ChildProcessWithoutNullStreams>(
60+
let printOutput = true;
61+
62+
return await new Promise<DisposableProcess>(
5963
(resolve, reject) => {
6064
const cmd = args.cmd;
6165
const fileName = args.fileName;
62-
let childProcess: cp.ChildProcessWithoutNullStreams;
66+
const processArgs = [
67+
`--silent`,
68+
`--slave`,
69+
`--no-save`,
70+
`--no-restore`,
71+
`-e`,
72+
cmd
73+
];
74+
const processOptions = {
75+
env: process.env
76+
};
77+
78+
let childProcess: DisposableProcess;
6379

6480
try {
65-
childProcess = cp.exec(cmd);
81+
childProcess = util.asDisposable(
82+
cp.spawn(
83+
`${this.rPath}`,
84+
processArgs,
85+
processOptions
86+
),
87+
() => {
88+
if (childProcess.kill('SIGKILL')) {
89+
rMarkdownOutput.appendLine('[VSC-R] terminating R process');
90+
printOutput = false;
91+
}
92+
}
93+
);
6694
progress.report({
6795
increment: 0,
6896
message: '0%'
@@ -81,9 +109,13 @@ export abstract class RMarkdownManager {
81109
childProcess.stdout.on('data',
82110
(data: Buffer) => {
83111
const dat = data.toString('utf8');
84-
this.rMarkdownOutput.appendLine(dat);
112+
if (printOutput) {
113+
this.rMarkdownOutput.appendLine(dat);
114+
115+
}
85116
const percentRegex = /[0-9]+(?=%)/g;
86117
const percentRegOutput = dat.match(percentRegex);
118+
87119
if (percentRegOutput) {
88120
for (const item of percentRegOutput) {
89121
const perc = Number(item);
@@ -108,7 +140,9 @@ export abstract class RMarkdownManager {
108140

109141
childProcess.stderr.on('data', (data: Buffer) => {
110142
const dat = data.toString('utf8');
111-
this.rMarkdownOutput.appendLine(dat);
143+
if (printOutput) {
144+
this.rMarkdownOutput.appendLine(dat);
145+
}
112146
});
113147

114148
childProcess.on('exit', (code, signal) => {
@@ -126,10 +160,10 @@ export abstract class RMarkdownManager {
126160
);
127161
}
128162

129-
protected async knitWithProgress(args: IKnitArgs): Promise<cp.ChildProcessWithoutNullStreams> {
130-
let childProcess: cp.ChildProcessWithoutNullStreams = undefined;
163+
protected async knitWithProgress(args: IKnitArgs): Promise<DisposableProcess> {
164+
let childProcess: DisposableProcess = undefined;
131165
await util.doWithProgress(async (token: vscode.CancellationToken, progress: vscode.Progress<unknown>) => {
132-
childProcess = await this.knitDocument(args, token, progress) as cp.ChildProcessWithoutNullStreams;
166+
childProcess = await this.knitDocument(args, token, progress) as DisposableProcess;
133167
},
134168
vscode.ProgressLocation.Notification,
135169
`Knitting ${args.fileName} ${args.rOutputFormat ? 'to ' + args.rOutputFormat : ''} `,
@@ -140,8 +174,8 @@ export abstract class RMarkdownManager {
140174
this.rMarkdownOutput.show(true);
141175
}
142176
// this can occur when a successfuly knitted document is later altered (while still being previewed) and subsequently fails to knit
143-
args?.onRejection ? args.onRejection(args.filePath, rejection) :
144-
rejection?.cp.kill('SIGKILL');
177+
args?.onRejection?.(args.filePath, rejection);
178+
rejection.cp?.dispose();
145179
});
146180
return childProcess;
147181
}

‎src/rmarkdown/preview.ts‎

Lines changed: 12 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
2-
import * as cp from 'child_process';
31
import * as vscode from 'vscode';
42
import * as fs from 'fs-extra';
53
import * as cheerio from 'cheerio';
@@ -11,11 +9,11 @@ import crypto = require('crypto');
119
import { config, readContent, setContext, escapeHtml, UriIcon, saveDocument, getRpath } from '../util';
1210
import { extensionContext, tmpDir } from '../extension';
1311
import { knitDir } from './knit';
14-
import { IKnitRejection, RMarkdownManager } from './manager';
12+
import { DisposableProcess, RMarkdownManager } from './manager';
1513

1614
class RMarkdownPreview extends vscode.Disposable {
1715
title: string;
18-
cp: cp.ChildProcessWithoutNullStreams;
16+
cp: DisposableProcess;
1917
panel: vscode.WebviewPanel;
2018
resourceViewColumn: vscode.ViewColumn;
2119
outputUri: vscode.Uri;
@@ -25,7 +23,7 @@ class RMarkdownPreview extends vscode.Disposable {
2523
autoRefresh: boolean;
2624
mtime: number;
2725

28-
constructor(title: string, cp: cp.ChildProcessWithoutNullStreams, panel: vscode.WebviewPanel,
26+
constructor(title: string, cp: DisposableProcess, panel: vscode.WebviewPanel,
2927
resourceViewColumn: vscode.ViewColumn, outputUri: vscode.Uri, filePath: string,
3028
RMarkdownPreviewManager: RMarkdownPreviewManager, useCodeTheme: boolean, autoRefresh: boolean) {
3129
super(() => {
@@ -268,7 +266,7 @@ export class RMarkdownPreviewManager extends RMarkdownManager {
268266
toUpdate?.cp?.kill('SIGKILL');
269267

270268
if (toUpdate) {
271-
const childProcess: cp.ChildProcessWithoutNullStreams | void = await this.previewDocument(previewUri, toUpdate.title).catch(() => {
269+
const childProcess: DisposableProcess | void = await this.previewDocument(previewUri, toUpdate.title).catch(() => {
272270
void vscode.window.showErrorMessage('There was an error in knitting the document. Please check the R Markdown output stream.');
273271
this.rMarkdownOutput.show(true);
274272
this.previewStore.delete(previewUri);
@@ -283,26 +281,25 @@ export class RMarkdownPreviewManager extends RMarkdownManager {
283281

284282
}
285283

286-
private async previewDocument(filePath: string, fileName?: string, viewer?: vscode.ViewColumn, currentViewColumn?: vscode.ViewColumn) {
284+
private async previewDocument(filePath: string, fileName?: string, viewer?: vscode.ViewColumn, currentViewColumn?: vscode.ViewColumn): Promise<DisposableProcess> {
287285
const knitWorkingDir = this.getKnitDir(knitDir, filePath);
288286
const knitWorkingDirText = knitWorkingDir ? `'${knitWorkingDir}'` : `NULL`;
289-
this.rPath = await getRpath(true);
287+
this.rPath = await getRpath();
290288

291289
const lim = '---vsc---';
292290
const re = new RegExp(`.*${lim}(.*)${lim}.*`, 'ms');
293291
const outputFile = path.join(tmpDir(), crypto.createHash('sha256').update(filePath).digest('hex') + '.html');
294292
const cmd = (
295-
`${this.rPath} --silent --slave --no-save --no-restore ` +
296-
`-e "knitr::opts_knit[['set']](root.dir = ${knitWorkingDirText})" ` +
297-
`-e "cat('${lim}', rmarkdown::render(` +
293+
`knitr::opts_knit[['set']](root.dir = ${knitWorkingDirText});` +
294+
`cat('${lim}', rmarkdown::render(` +
298295
`'${filePath.replace(/\\/g, '/')}',` +
299296
`output_format = rmarkdown::html_document(),` +
300297
`output_file = '${outputFile.replace(/\\/g, '/')}',` +
301298
`intermediates_dir = '${tmpDir().replace(/\\/g, '/')}'), '${lim}',` +
302-
`sep='')"`
299+
`sep='')`
303300
);
304301

305-
const callback = (dat: string, childProcess: cp.ChildProcessWithoutNullStreams) => {
302+
const callback = (dat: string, childProcess: DisposableProcess) => {
306303
const outputUrl = re.exec(dat)?.[0]?.replace(re, '$1');
307304
if (outputUrl) {
308305
if (viewer !== undefined) {
@@ -322,11 +319,9 @@ export class RMarkdownPreviewManager extends RMarkdownManager {
322319
return false;
323320
};
324321

325-
const onRejected = (filePath: string, rejection: IKnitRejection) => {
322+
const onRejected = (filePath: string) => {
326323
if (this.previewStore.has(filePath)) {
327324
this.previewStore.delete(filePath);
328-
} else {
329-
rejection.cp.kill('SIGKILL');
330325
}
331326
};
332327

@@ -342,7 +337,7 @@ export class RMarkdownPreviewManager extends RMarkdownManager {
342337
);
343338
}
344339

345-
private openPreview(outputUri: vscode.Uri, filePath: string, title: string, cp: cp.ChildProcessWithoutNullStreams, viewer: vscode.ViewColumn, resourceViewColumn: vscode.ViewColumn, autoRefresh: boolean): void {
340+
private openPreview(outputUri: vscode.Uri, filePath: string, title: string, cp: DisposableProcess, viewer: vscode.ViewColumn, resourceViewColumn: vscode.ViewColumn, autoRefresh: boolean): void {
346341

347342
const panel = vscode.window.createWebviewPanel(
348343
'previewRmd',

‎src/util.ts‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -371,3 +371,20 @@ export class UriIcon {
371371
this.light = vscode.Uri.file(path.join(extIconPath, 'light', id + '.svg'));
372372
}
373373
}
374+
375+
/**
376+
* As Disposable.
377+
*
378+
* Create a dispose method for any given object, and push it to the
379+
* extension subscriptions array
380+
*
381+
* @param {T} toDispose - the object to add dispose to
382+
* @param {Function} disposeFunction - the method called when the object is disposed
383+
* @returns returned object is considered types T and vscode.Disposable
384+
*/
385+
export function asDisposable<T>(toDispose: T, disposeFunction: (...args: unknown[]) => unknown): T & vscode.Disposable {
386+
type disposeType = T & vscode.Disposable;
387+
(toDispose as disposeType).dispose = () => disposeFunction();
388+
extensionContext.subscriptions.push(toDispose as disposeType);
389+
return toDispose as disposeType;
390+
}

0 commit comments

Comments
 (0)