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>
microsoft · Sep 9, 2024 · a56dfec · a56dfec
1 parent 98dccbd
commit a56dfec
Show file tree

Hide file tree

Showing 7 changed files with 125 additions and 47 deletions.
diff --git a/node/CHANGELOG.md b/node/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## 4.x
 
+## 4.17.2
+
+- Fix ToolRunner stdline/errline events buffering - [#1055](https://github.com/microsoft/azure-pipelines-task-lib/pull/1055)
+
 ## 4.17.1
 
 - Fix debug logs inside user commands - [#1064](https://github.com/microsoft/azure-pipelines-task-lib/pull/1064)

diff --git a/node/package-lock.json b/node/package-lock.json
diff --git a/node/package.json b/node/package.json
@@ -1,6 +1,6 @@
 {
   "name": "azure-pipelines-task-lib",
-  "version": "4.17.1",
+  "version": "4.17.2",
   "description": "Azure Pipelines Task SDK",
   "main": "./task.js",
   "typings": "./task.d.ts",

diff --git a/node/test/scripts/write-bufferedoutput.js b/node/test/scripts/write-bufferedoutput.js
@@ -0,0 +1,12 @@
+const os = require('os');
+
+const stdout = process.stdout;
+const stderr = process.stderr;
+
+stdout.write('stdline 1' + os.EOL,
+    () => stdout.write('stdline 2',
+        () => stdout.write(os.EOL + 'stdline 3')));
+
+stderr.write('errline 1' + os.EOL,
+    () => stderr.write('errline 2',
+        () => stderr.write(os.EOL + 'errline 3')));
diff --git a/node/test/toolrunnerTestsWithExecAsync.ts b/node/test/toolrunnerTestsWithExecAsync.ts
@@ -112,6 +112,36 @@ describe('Toolrunner Tests With ExecAsync', function () {
                 });
         }
     })
+    it('Writes correct output line events', async function () {
+        const scriptPath = path.join(__dirname, 'scripts', 'write-bufferedoutput.js');
+        const node = tl.tool(tl.which('node', true));
+        node.arg(scriptPath);
+
+        const stdlines = [];
+        const errlines = [];
+
+        node.on('stdline', function (line) {
+            stdlines.push(line);
+        });
+
+        node.on('errline', function (line) {
+            errlines.push(line);
+        });
+
+        const code = await node.execAsync({
+            cwd: __dirname,
+            env: {},
+            silent: false,
+            failOnStdErr: false,
+            ignoreReturnCode: false,
+            outStream: testutil.getNullStream(),
+            errStream: testutil.getNullStream()
+        })
+
+        assert.deepStrictEqual(code, 0, 'return code of cmd should be 0');
+        assert.deepStrictEqual(stdlines, ['stdline 1', 'stdline 2', 'stdline 3'], 'should have emitted stdlines');
+        assert.deepStrictEqual(errlines, ['errline 1', 'errline 2', 'errline 3'], 'should have emitted errlines');
+    })
     it('Execs with stdout', function (done) {
         this.timeout(10000);
 

diff --git a/node/test/toolrunnertests.ts b/node/test/toolrunnertests.ts
@@ -202,6 +202,36 @@ describe('Toolrunner Tests', function () {
                 });
         }
     })
+    it('Writes correct output line events', async function () {
+        const scriptPath = path.join(__dirname, 'scripts', 'write-bufferedoutput.js');
+        const node = tl.tool(tl.which('node', true));
+        node.arg(scriptPath);
+
+        const stdlines = [];
+        const errlines = [];
+
+        node.on('stdline', function (line) {
+            stdlines.push(line);
+        });
+
+        node.on('errline', function (line) {
+            errlines.push(line);
+        });
+
+        const code = await node.exec({
+            cwd: __dirname,
+            env: {},
+            silent: false,
+            failOnStdErr: false,
+            ignoreReturnCode: false,
+            outStream: testutil.getNullStream(),
+            errStream: testutil.getNullStream()
+        })
+
+        assert.deepStrictEqual(code, 0, 'return code of cmd should be 0');
+        assert.deepStrictEqual(stdlines, ['stdline 1', 'stdline 2', 'stdline 3'], 'should have emitted stdlines');
+        assert.deepStrictEqual(errlines, ['errline 1', 'errline 2', 'errline 3'], 'should have emitted errlines');
+    })
     it('Execs with stdout', function (done) {
         this.timeout(10000);
 

diff --git a/node/toolrunner.ts b/node/toolrunner.ts
@@ -2,7 +2,6 @@ import Q = require('q');
 import os = require('os');
 import events = require('events');
 import child = require('child_process');
-import stream = require('stream');
 import im = require('./internal');
 import fs = require('fs');
 
@@ -194,27 +193,27 @@ export class ToolRunner extends events.EventEmitter {
         return cmd;
     }
 
-    private _processLineBuffer(data: Buffer, strBuffer: string, onLine: (line: string) => void): void {
+    private _processLineBuffer(data: Buffer, buffer: string, onLine: (line: string) => void): string {
+        let newBuffer = buffer + data.toString();
+
         try {
-            var s = strBuffer + data.toString();
-            var n = s.indexOf(os.EOL);
+            let eolIndex = newBuffer.indexOf(os.EOL);
 
-            while (n > -1) {
-                var line = s.substring(0, n);
+            while (eolIndex > -1) {
+                const line = newBuffer.substring(0, eolIndex);
                 onLine(line);
 
                 // the rest of the string ...
-                s = s.substring(n + os.EOL.length);
-                n = s.indexOf(os.EOL);
+                newBuffer = newBuffer.substring(eolIndex + os.EOL.length);
+                eolIndex = newBuffer.indexOf(os.EOL);
             }
-
-            strBuffer = s;
         }
         catch (err) {
             // streaming lines to console is best effort.  Don't fail a build.
             this._debug('error processing line');
         }
 
+        return newBuffer;
     }
 
     /**
@@ -728,20 +727,20 @@ export class ToolRunner extends events.EventEmitter {
                 }
             });
 
-            var stdbuffer: string = '';
+            let stdLineBuffer = '';
             cp.stdout?.on('data', (data: Buffer) => {
                 this.emit('stdout', data);
 
                 if (!optionsNonNull.silent) {
                     optionsNonNull.outStream!.write(data);
                 }
 
-                this._processLineBuffer(data, stdbuffer, (line: string) => {
+                stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
                     this.emit('stdline', line);
                 });
             });
 
-            var errbuffer: string = '';
+            let errLineBuffer = '';
             cp.stderr?.on('data', (data: Buffer) => {
                 this.emit('stderr', data);
 
@@ -751,7 +750,7 @@ export class ToolRunner extends events.EventEmitter {
                     s.write(data);
                 }
 
-                this._processLineBuffer(data, errbuffer, (line: string) => {
+                errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
                     this.emit('errline', line);
                 });
             });
@@ -769,12 +768,12 @@ export class ToolRunner extends events.EventEmitter {
                 this._debug('rc:' + code);
                 returnCode = code;
 
-                if (stdbuffer.length > 0) {
-                    this.emit('stdline', stdbuffer);
+                if (stdLineBuffer.length > 0) {
+                    this.emit('stdline', stdLineBuffer);
                 }
 
-                if (errbuffer.length > 0) {
-                    this.emit('errline', errbuffer);
+                if (errLineBuffer.length > 0) {
+                    this.emit('errline', errLineBuffer);
                 }
 
                 if (code != 0 && !optionsNonNull.ignoreReturnCode) {
@@ -926,20 +925,20 @@ export class ToolRunner extends events.EventEmitter {
             }
         });
 
-        var stdbuffer: string = '';
+        let stdLineBuffer = '';
         cp.stdout?.on('data', (data: Buffer) => {
             this.emit('stdout', data);
 
             if (!optionsNonNull.silent) {
                 optionsNonNull.outStream!.write(data);
             }
 
-            this._processLineBuffer(data, stdbuffer, (line: string) => {
+            stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
                 this.emit('stdline', line);
             });
         });
 
-        var errbuffer: string = '';
+        let errLineBuffer = '';
         cp.stderr?.on('data', (data: Buffer) => {
             this.emit('stderr', data);
 
@@ -949,7 +948,7 @@ export class ToolRunner extends events.EventEmitter {
                 s.write(data);
             }
 
-            this._processLineBuffer(data, errbuffer, (line: string) => {
+            errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
                 this.emit('errline', line);
             });
         });
@@ -967,12 +966,12 @@ export class ToolRunner extends events.EventEmitter {
             this._debug('rc:' + code);
             returnCode = code;
 
-            if (stdbuffer.length > 0) {
-                this.emit('stdline', stdbuffer);
+            if (stdLineBuffer.length > 0) {
+                this.emit('stdline', stdLineBuffer);
             }
 
-            if (errbuffer.length > 0) {
-                this.emit('errline', errbuffer);
+            if (errLineBuffer.length > 0) {
+                this.emit('errline', errLineBuffer);
             }
 
             if (code != 0 && !optionsNonNull.ignoreReturnCode) {
@@ -1100,16 +1099,19 @@ export class ToolRunner extends events.EventEmitter {
             this._debug(message);
         });
 
-        var stdbuffer: string = '';
-        var errbuffer: string = '';
-        const emitDoneEvent = function (resolve, reject) {
+        let stdLineBuffer = '';
+        let errLineBuffer = '';
+        const emitDoneEvent = (
+            resolve: (code: number) => void,
+            reject: (error: Error) => void
+        ) => {
             state.on('done', (error: Error, exitCode: number) => {
-                if (stdbuffer.length > 0) {
-                    this.emit('stdline', stdbuffer);
+                if (stdLineBuffer.length > 0) {
+                    this.emit('stdline', stdLineBuffer);
                 }
 
-                if (errbuffer.length > 0) {
-                    this.emit('errline', errbuffer);
+                if (errLineBuffer.length > 0) {
+                    this.emit('errline', errLineBuffer);
                 }
 
                 if (cp) {
@@ -1126,7 +1128,7 @@ export class ToolRunner extends events.EventEmitter {
         }
 
         // Edge case when the node itself cant's spawn and emit event
-        let cp;
+        let cp: child.ChildProcess;
         try {
             cp = child.spawn(this._getSpawnFileName(options), this._getSpawnArgs(optionsNonNull), this._getSpawnOptions(options));
         } catch (error) {
@@ -1156,7 +1158,7 @@ export class ToolRunner extends events.EventEmitter {
                 optionsNonNull.outStream!.write(data);
             }
 
-            this._processLineBuffer(data, stdbuffer, (line: string) => {
+            stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
                 this.emit('stdline', line);
             });
         });
@@ -1170,7 +1172,7 @@ export class ToolRunner extends events.EventEmitter {
                 s.write(data);
             }
 
-            this._processLineBuffer(data, errbuffer, (line: string) => {
+            errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
                 this.emit('errline', line);
             });
         });
@@ -1234,15 +1236,15 @@ export class ToolRunner extends events.EventEmitter {
             this._debug(message);
         });
 
-        var stdbuffer: string = '';
-        var errbuffer: string = '';
+        let stdLineBuffer = '';
+        let errLineBuffer = '';
         state.on('done', (error: Error, exitCode: number) => {
-            if (stdbuffer.length > 0) {
-                this.emit('stdline', stdbuffer);
+            if (stdLineBuffer.length > 0) {
+                this.emit('stdline', stdLineBuffer);
             }
 
-            if (errbuffer.length > 0) {
-                this.emit('errline', errbuffer);
+            if (errLineBuffer.length > 0) {
+                this.emit('errline', errLineBuffer);
             }
 
             if (cp) {
@@ -1259,7 +1261,7 @@ export class ToolRunner extends events.EventEmitter {
 
 
         // Edge case when the node itself cant's spawn and emit event
-        let cp;
+        let cp: child.ChildProcess;
         try {
             cp = child.spawn(this._getSpawnFileName(options), this._getSpawnArgs(optionsNonNull), this._getSpawnOptions(options));
         } catch (error) {
@@ -1288,7 +1290,7 @@ export class ToolRunner extends events.EventEmitter {
                 optionsNonNull.outStream!.write(data);
             }
 
-            this._processLineBuffer(data, stdbuffer, (line: string) => {
+            stdLineBuffer = this._processLineBuffer(data, stdLineBuffer, (line: string) => {
                 this.emit('stdline', line);
             });
         });
@@ -1303,7 +1305,7 @@ export class ToolRunner extends events.EventEmitter {
                 s.write(data);
             }
 
-            this._processLineBuffer(data, errbuffer, (line: string) => {
+            errLineBuffer = this._processLineBuffer(data, errLineBuffer, (line: string) => {
                 this.emit('errline', line);
             });
         });