stream: fix Writable subclass instanceof checks

addaleax · evanlucas · commit 8c4fab0a28ed · 2016-10-14T16:29:51.000-05:00
2a4b068 introduced a regression in where checking `instanceof` would fail for `Writable` subclasses inside the subclass constructor, i.e. before `Writable()` was called. Also, calling `null instanceof Writable` or `undefined instanceof Writable` would fail due to accessing the `_writableState` property of the target object. This fixes these problems. PR-URL: #9088 Ref: #8834 (comment) Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>
diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
@@ -141,9 +141,10 @@ if (typeof Symbol === 'function' && Symbol.hasInstance) {
   realHasInstance = Function.prototype[Symbol.hasInstance];
   Object.defineProperty(Writable, Symbol.hasInstance, {
     value: function(object) {
-      // Trying to move the `realHasInstance` from the Writable constructor
-      // to here will break the Node.js LazyTransform implementation.
-      return object._writableState instanceof WritableState;
+      if (realHasInstance.call(this, object))
+        return true;
+
+      return object && object._writableState instanceof WritableState;
     }
   });
 } else {
@@ -156,6 +157,10 @@ function Writable(options) {
   // Writable ctor is applied to Duplexes, too.
   // `realHasInstance` is necessary because using plain `instanceof`
   // would return false, as no `_writableState` property is attached.
+
+  // Trying to use the custom `instanceof` for Writable here will also break the
+  // Node.js LazyTransform implementation, which has a non-trivial getter for
+  // `_writableState` that would lead to infinite recursion.
   if (!(realHasInstance.call(Writable, this)) &&
       !(this instanceof Stream.Duplex)) {
     return new Writable(options);
diff --git a/test/parallel/test-stream-inheritance.js b/test/parallel/test-stream-inheritance.js
@@ -27,3 +27,19 @@ assert.ok(!(readable instanceof Transform));
 assert.ok(!(writable instanceof Transform));
 assert.ok(!(duplex instanceof Transform));
 assert.ok(transform instanceof Transform);
+
+assert.ok(!(null instanceof Writable));
+assert.ok(!(undefined instanceof Writable));
+
+// Simple inheritance check for `Writable` works fine in a subclass constructor.
+function CustomWritable() {
+  assert.ok(this instanceof Writable, 'inherits from Writable');
+  assert.ok(this instanceof CustomWritable, 'inherits from CustomWritable');
+}
+
+Object.setPrototypeOf(CustomWritable, Writable);
+Object.setPrototypeOf(CustomWritable.prototype, Writable.prototype);
+
+new CustomWritable();
+
+assert.throws(CustomWritable, /AssertionError: inherits from Writable/);
