Skip to content

Commit a56dfec

Browse files
localconstEvanCahill
localconst
and
EvanCahill
authored
Fix ToolRunner stdline/errline events buffering (#1055)
* Add tests for stdline and errline * Correctly buffer line data in ToolRunner The _processLineBuffer method is incorrectly buffering line data since the buffer is being passed as a string and thus changes to the buffer preformed in the method are not persisted. The fix is to use an object with a string property as the buffer. * Fix context of emitDoneEvent * Update buffering logic * format code * remove unneded import * Add types * Update tests * Bump version + add changelog * rename buffer variables * Rename test & script file --------- Co-authored-by: Evan Cahill <hacahill@microsoft.com>
1 parent 98dccbd commit a56dfec

File tree

7 files changed

+125
-47
lines changed

7 files changed

+125
-47
lines changed

‎node/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
## 4.x
44

5+
## 4.17.2
6+
7+
- Fix ToolRunner stdline/errline events buffering - [#1055](https://github.com/microsoft/azure-pipelines-task-lib/pull/1055)
8+
59
## 4.17.1
610

711
- Fix debug logs inside user commands - [#1064](https://github.com/microsoft/azure-pipelines-task-lib/pull/1064)

‎node/package-lock.json

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎node/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
{
22
"name": "azure-pipelines-task-lib",
3-
"version": "4.17.1",
3+
"version": "4.17.2",
44
"description": "Azure Pipelines Task SDK",
55
"main": "./task.js",
66
"typings": "./task.d.ts",

‎node/test/scripts/write-bufferedoutput.js

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
const os = require('os');
2+
3+
const stdout = process.stdout;
4+
const stderr = process.stderr;
5+
6+
stdout.write('stdline 1' + os.EOL,
7+
() => stdout.write('stdline 2',
8+
() => stdout.write(os.EOL + 'stdline 3')));
9+
10+
stderr.write('errline 1' + os.EOL,
11+
() => stderr.write('errline 2',
12+
() => stderr.write(os.EOL + 'errline 3')));

‎node/test/toolrunnerTestsWithExecAsync.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,36 @@ describe('Toolrunner Tests With ExecAsync', function () {
112112
});
113113
}
114114
})
115+
it('Writes correct output line events', async function () {
116+
const scriptPath = path.join(__dirname, 'scripts', 'write-bufferedoutput.js');
117+
const node = tl.tool(tl.which('node', true));
118+
node.arg(scriptPath);
119+
120+
const stdlines = [];
121+
const errlines = [];
122+
123+
node.on('stdline', function (line) {
124+
stdlines.push(line);
125+
});
126+
127+
node.on('errline', function (line) {
128+
errlines.push(line);
129+
});
130+
131+
const code = await node.execAsync({
132+
cwd: __dirname,
133+
env: {},
134+
silent: false,
135+
failOnStdErr: false,
136+
ignoreReturnCode: false,
137+
outStream: testutil.getNullStream(),
138+
errStream: testutil.getNullStream()
139+
})
140+
141+
assert.deepStrictEqual(code, 0, 'return code of cmd should be 0');
142+
assert.deepStrictEqual(stdlines, ['stdline 1', 'stdline 2', 'stdline 3'], 'should have emitted stdlines');
143+
assert.deepStrictEqual(errlines, ['errline 1', 'errline 2', 'errline 3'], 'should have emitted errlines');
144+
})
115145
it('Execs with stdout', function (done) {
116146
this.timeout(10000);
117147

‎node/test/toolrunnertests.ts

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,36 @@ describe('Toolrunner Tests', function () {
202202
});
203203
}
204204
})
205+
it('Writes correct output line events', async function () {
206+
const scriptPath = path.join(__dirname, 'scripts', 'write-bufferedoutput.js');
207+
const node = tl.tool(tl.which('node', true));
208+
node.arg(scriptPath);
209+
210+
const stdlines = [];
211+
const errlines = [];
212+
213+
node.on('stdline', function (line) {
214+
stdlines.push(line);
215+
});
216+
217+
node.on('errline', function (line) {
218+
errlines.push(line);
219+
});
220+
221+
const code = await node.exec({
222+
cwd: __dirname,
223+
env: {},
224+
silent: false,
225+
failOnStdErr: false,
226+
ignoreReturnCode: false,
227+
outStream: testutil.getNullStream(),
228+
errStream: testutil.getNullStream()
229+
})
230+
231+
assert.deepStrictEqual(code, 0, 'return code of cmd should be 0');
232+
assert.deepStrictEqual(stdlines, ['stdline 1', 'stdline 2', 'stdline 3'], 'should have emitted stdlines');
233+
assert.deepStrictEqual(errlines, ['errline 1', 'errline 2', 'errline 3'], 'should have emitted errlines');
234+
})
205235
it('Execs with stdout', function (done) {
206236
this.timeout(10000);
207237

‎node/toolrunner.ts

Lines changed: 47 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ import Q = require('q');
22
import os = require('os');
33
import events = require('events');
44
import child = require('child_process');
5-
import stream = require('stream');
65
import im = require('./internal');
76
import fs = require('fs');
87

@@ -194,27 +193,27 @@ export class ToolRunner extends events.EventEmitter {
194193
return cmd;
195194
}
196195

197-
private _processLineBuffer(data: Buffer, strBuffer: string, onLine: (line: string) => void): void {
196+
private _processLineBuffer(data: Buffer, buffer: string, onLine: (line: string) => void): string {
197+
let newBuffer = buffer + data.toString();
198+
198199
try {
199-
var s = strBuffer + data.toString();
200-
var n = s.indexOf(os.EOL);
200+
let eolIndex = newBuffer.indexOf(os.EOL);
201201

202-
while (n > -1) {
203-
var line = s.substring(0, n);
202+
while (eolIndex > -1) {
203+
const line = newBuffer.substring(0, eolIndex);
204204
onLine(line);
205205

206206
// the rest of the string ...
207-
s = s.substring(n + os.EOL.length);
208-
n = s.indexOf(os.EOL);
207+
newBuffer = newBuffer.substring(eolIndex + os.EOL.length);
208+
eolIndex = newBuffer.indexOf(os.EOL);
209209
}
210-
211-
strBuffer = s;
212210
}
213211
catch (err) {
214212
// streaming lines to console is best effort. Don't fail a build.
215213
this._debug('error processing line');
216214
}
217215

216+
return newBuffer;
218217
}
219218

220219
/**
@@ -728,20 +727,20 @@ export class ToolRunner extends events.EventEmitter {
728727
}
729728
});
730729

731-
var stdbuffer: string = '';
730+
let stdLineBuffer = '';
732731
cp.stdout?.on('data', (data: Buffer) => {
733732
this.emit('stdout', data);
734733

735734
if (!optionsNonNull.silent) {
736735
optionsNonNull.outStream!.write(data);
737736
}
738737

739-
this._processLineBuffer(data, stdbuffer, (line: string) => {
738+
stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
740739
this.emit('stdline', line);
741740
});
742741
});
743742

744-
var errbuffer: string = '';
743+
let errLineBuffer = '';
745744
cp.stderr?.on('data', (data: Buffer) => {
746745
this.emit('stderr', data);
747746

@@ -751,7 +750,7 @@ export class ToolRunner extends events.EventEmitter {
751750
s.write(data);
752751
}
753752

754-
this._processLineBuffer(data, errbuffer, (line: string) => {
753+
errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
755754
this.emit('errline', line);
756755
});
757756
});
@@ -769,12 +768,12 @@ export class ToolRunner extends events.EventEmitter {
769768
this._debug('rc:' + code);
770769
returnCode = code;
771770

772-
if (stdbuffer.length > 0) {
773-
this.emit('stdline', stdbuffer);
771+
if (stdLineBuffer.length > 0) {
772+
this.emit('stdline', stdLineBuffer);
774773
}
775774

776-
if (errbuffer.length > 0) {
777-
this.emit('errline', errbuffer);
775+
if (errLineBuffer.length > 0) {
776+
this.emit('errline', errLineBuffer);
778777
}
779778

780779
if (code != 0 && !optionsNonNull.ignoreReturnCode) {
@@ -926,20 +925,20 @@ export class ToolRunner extends events.EventEmitter {
926925
}
927926
});
928927

929-
var stdbuffer: string = '';
928+
let stdLineBuffer = '';
930929
cp.stdout?.on('data', (data: Buffer) => {
931930
this.emit('stdout', data);
932931

933932
if (!optionsNonNull.silent) {
934933
optionsNonNull.outStream!.write(data);
935934
}
936935

937-
this._processLineBuffer(data, stdbuffer, (line: string) => {
936+
stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
938937
this.emit('stdline', line);
939938
});
940939
});
941940

942-
var errbuffer: string = '';
941+
let errLineBuffer = '';
943942
cp.stderr?.on('data', (data: Buffer) => {
944943
this.emit('stderr', data);
945944

@@ -949,7 +948,7 @@ export class ToolRunner extends events.EventEmitter {
949948
s.write(data);
950949
}
951950

952-
this._processLineBuffer(data, errbuffer, (line: string) => {
951+
errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
953952
this.emit('errline', line);
954953
});
955954
});
@@ -967,12 +966,12 @@ export class ToolRunner extends events.EventEmitter {
967966
this._debug('rc:' + code);
968967
returnCode = code;
969968

970-
if (stdbuffer.length > 0) {
971-
this.emit('stdline', stdbuffer);
969+
if (stdLineBuffer.length > 0) {
970+
this.emit('stdline', stdLineBuffer);
972971
}
973972

974-
if (errbuffer.length > 0) {
975-
this.emit('errline', errbuffer);
973+
if (errLineBuffer.length > 0) {
974+
this.emit('errline', errLineBuffer);
976975
}
977976

978977
if (code != 0 && !optionsNonNull.ignoreReturnCode) {
@@ -1100,16 +1099,19 @@ export class ToolRunner extends events.EventEmitter {
11001099
this._debug(message);
11011100
});
11021101

1103-
var stdbuffer: string = '';
1104-
var errbuffer: string = '';
1105-
const emitDoneEvent = function (resolve, reject) {
1102+
let stdLineBuffer = '';
1103+
let errLineBuffer = '';
1104+
const emitDoneEvent = (
1105+
resolve: (code: number) => void,
1106+
reject: (error: Error) => void
1107+
) => {
11061108
state.on('done', (error: Error, exitCode: number) => {
1107-
if (stdbuffer.length > 0) {
1108-
this.emit('stdline', stdbuffer);
1109+
if (stdLineBuffer.length > 0) {
1110+
this.emit('stdline', stdLineBuffer);
11091111
}
11101112

1111-
if (errbuffer.length > 0) {
1112-
this.emit('errline', errbuffer);
1113+
if (errLineBuffer.length > 0) {
1114+
this.emit('errline', errLineBuffer);
11131115
}
11141116

11151117
if (cp) {
@@ -1126,7 +1128,7 @@ export class ToolRunner extends events.EventEmitter {
11261128
}
11271129

11281130
// Edge case when the node itself cant's spawn and emit event
1129-
let cp;
1131+
let cp: child.ChildProcess;
11301132
try {
11311133
cp = child.spawn(this._getSpawnFileName(options), this._getSpawnArgs(optionsNonNull), this._getSpawnOptions(options));
11321134
} catch (error) {
@@ -1156,7 +1158,7 @@ export class ToolRunner extends events.EventEmitter {
11561158
optionsNonNull.outStream!.write(data);
11571159
}
11581160

1159-
this._processLineBuffer(data, stdbuffer, (line: string) => {
1161+
stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
11601162
this.emit('stdline', line);
11611163
});
11621164
});
@@ -1170,7 +1172,7 @@ export class ToolRunner extends events.EventEmitter {
11701172
s.write(data);
11711173
}
11721174

1173-
this._processLineBuffer(data, errbuffer, (line: string) => {
1175+
errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
11741176
this.emit('errline', line);
11751177
});
11761178
});
@@ -1234,15 +1236,15 @@ export class ToolRunner extends events.EventEmitter {
12341236
this._debug(message);
12351237
});
12361238

1237-
var stdbuffer: string = '';
1238-
var errbuffer: string = '';
1239+
let stdLineBuffer = '';
1240+
let errLineBuffer = '';
12391241
state.on('done', (error: Error, exitCode: number) => {
1240-
if (stdbuffer.length > 0) {
1241-
this.emit('stdline', stdbuffer);
1242+
if (stdLineBuffer.length > 0) {
1243+
this.emit('stdline', stdLineBuffer);
12421244
}
12431245

1244-
if (errbuffer.length > 0) {
1245-
this.emit('errline', errbuffer);
1246+
if (errLineBuffer.length > 0) {
1247+
this.emit('errline', errLineBuffer);
12461248
}
12471249

12481250
if (cp) {
@@ -1259,7 +1261,7 @@ export class ToolRunner extends events.EventEmitter {
12591261

12601262

12611263
// Edge case when the node itself cant's spawn and emit event
1262-
let cp;
1264+
let cp: child.ChildProcess;
12631265
try {
12641266
cp = child.spawn(this._getSpawnFileName(options), this._getSpawnArgs(optionsNonNull), this._getSpawnOptions(options));
12651267
} catch (error) {
@@ -1288,7 +1290,7 @@ export class ToolRunner extends events.EventEmitter {
12881290
optionsNonNull.outStream!.write(data);
12891291
}
12901292

1291-
this._processLineBuffer(data, stdbuffer, (line: string) => {
1293+
stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
12921294
this.emit('stdline', line);
12931295
});
12941296
});
@@ -1303,7 +1305,7 @@ export class ToolRunner extends events.EventEmitter {
13031305
s.write(data);
13041306
}
13051307

1306-
this._processLineBuffer(data, errbuffer, (line: string) => {
1308+
errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
13071309
this.emit('errline', line);
13081310
});
13091311
});

0 commit comments

Comments
 (0)