fix: node 14+ changes how multiple destroy() calls work (#1153)

feywind · alexander-fenster · web-flow · commit e42174987ec4 · 2020-11-18T07:47:35.000-08:00
* fix: node 14+ changes how multiple destroy() calls work, and this relied on node &lt;14 behavior. also removed some node &lt;8 cruft.

* chore: actually run on Node 14 and 15

* chore: no scripts on first install

* chore: finally make it work

* style: remove extraneous underscores

* style: remove now-extraneous import from message-stream tests

Co-authored-by: Alexander Fenster &lt;github@fenster.name&gt;
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -9,7 +9,7 @@ jobs:
     runs-on: ubuntu-latest
     strategy:
       matrix:
-        node: [10, 12, 13]
+        node: [10, 12, 14, 15]
     steps:
       - uses: actions/checkout@v2
       - uses: actions/setup-node@v1
@@ -19,7 +19,9 @@ jobs:
       # The first installation step ensures that all of our production
       # dependencies work on the given Node.js version, this helps us find
       # dependencies that don't match our engines field:
-      - run: npm install --production --engine-strict
+      - run: npm install --production --engine-strict --ignore-scripts --no-package-lock
+      # Clean up the production install, before installing dev/production:
+      - run: rm -rf node_modules
       - run: npm install
       - run: npm test
       - name: coverage
@@ -33,7 +35,7 @@ jobs:
       - uses: actions/checkout@v2
       - uses: actions/setup-node@v1
         with:
-          node-version: 12
+          node-version: 14
       - run: npm install
       - run: npm test
       - name: coverage
@@ -47,7 +49,7 @@ jobs:
       - uses: actions/checkout@v2
       - uses: actions/setup-node@v1
         with:
-          node-version: 12
+          node-version: 14
       - run: npm install
       - run: npm run lint
   docs:
@@ -56,6 +58,6 @@ jobs:
       - uses: actions/checkout@v2
       - uses: actions/setup-node@v1
         with:
-          node-version: 12
+          node-version: 14
       - run: npm install
       - run: npm run docs-test
diff --git a/src/message-stream.ts b/src/message-stream.ts
@@ -127,7 +127,6 @@ export interface MessageStreamOptions {
  * @param {MessageStreamOptions} [options] The message stream options.
  */
 export class MessageStream extends PassThrough {
-  destroyed: boolean;
   private _keepAliveHandle: NodeJS.Timer;
   private _fillHandle?: NodeJS.Timer;
   private _options: MessageStreamOptions;
@@ -139,7 +138,6 @@ export class MessageStream extends PassThrough {
 
     super({objectMode: true, highWaterMark: options.highWaterMark});
 
-    this.destroyed = false;
     this._options = options;
     this._retrier = new PullRetry();
     this._streams = new Map();
@@ -156,14 +154,24 @@ export class MessageStream extends PassThrough {
   /**
    * Destroys the stream and any underlying streams.
    *
-   * @param {error?} err An error to emit, if any.
+   * @param {error?} error An error to emit, if any.
    * @private
    */
-  destroy(err?: Error): void {
+  destroy(error?: Error | null): void {
+    // We can't assume Node has taken care of this in <14.
     if (this.destroyed) {
       return;
     }
-
+    super.destroy(error ? error : undefined);
+  }
+  /**
+   * Destroys the stream and any underlying streams.
+   *
+   * @param {error?} error An error to emit, if any.
+   * @param {Function} callback Callback for completion of any destruction.
+   * @private
+   */
+  _destroy(error: Error | null, callback: (error: Error | null) => void): void {
     this.destroyed = true;
     clearInterval(this._keepAliveHandle);
 
@@ -172,16 +180,7 @@ export class MessageStream extends PassThrough {
       stream.cancel();
     }
 
-    if (typeof super.destroy === 'function') {
-      return super.destroy(err);
-    }
-
-    process.nextTick(() => {
-      if (err) {
-        this.emit('error', err);
-      }
-      this.emit('close');
-    });
+    callback(error);
   }
   /**
    * Adds a StreamingPull stream to the combined stream.
diff --git a/test/message-stream.ts b/test/message-stream.ts
@@ -15,7 +15,7 @@
  */
 
 import * as assert from 'assert';
-import {describe, it, before, beforeEach, afterEach, after} from 'mocha';
+import {describe, it, before, beforeEach, afterEach} from 'mocha';
 import {grpc} from 'google-gax';
 import * as proxyquire from 'proxyquire';
 import * as sinon from 'sinon';
@@ -302,13 +302,7 @@ describe('MessageStream', () => {
 
   describe('destroy', () => {
     it('should noop if already destroyed', done => {
-      sandbox
-        .stub(FakePassThrough.prototype, 'destroy')
-        .callsFake(function (this: Duplex) {
-          if (this === messageStream) {
-            done();
-          }
-        });
+      messageStream.on('close', done);
 
       messageStream.destroy();
       messageStream.destroy();
@@ -350,36 +344,6 @@ describe('MessageStream', () => {
         assert.strictEqual(stub.callCount, 1);
       });
     });
-
-    describe('without native destroy', () => {
-      let destroy: (err?: Error) => void;
-
-      before(() => {
-        destroy = FakePassThrough.prototype.destroy;
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        FakePassThrough.prototype.destroy = false as any;
-      });
-
-      after(() => {
-        FakePassThrough.prototype.destroy = destroy;
-      });
-
-      it('should emit close', done => {
-        messageStream.on('close', done);
-        messageStream.destroy();
-      });
-
-      it('should emit an error if present', done => {
-        const fakeError = new Error('err');
-
-        messageStream.on('error', err => {
-          assert.strictEqual(err, fakeError);
-          done();
-        });
-
-        messageStream.destroy(fakeError);
-      });
-    });
   });
 
   describe('pull stream lifecycle', () => {
