From 8b90814b85282acea140c72b59bd6ec016985668 Mon Sep 17 00:00:00 2001 From: Anna Henningsen Date: Fri, 14 Oct 2016 00:09:30 +0200 Subject: [PATCH] stream: fix `Writable` subclass instanceof checks 2a4b068acaa160 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: https://github.com/nodejs/node/pull/9088 Ref: https://github.com/nodejs/node/pull/8834#issuecomment-253640692 Reviewed-By: Ilkka Myller Reviewed-By: Jeremiah Senkpiel Reviewed-By: James M Snell Reviewed-By: Matteo Collina Reviewed-By: Evan Lucas --- lib/_stream_writable.js | 11 ++++++++--- test/parallel/test-stream-inheritance.js | 16 ++++++++++++++++ 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index f6c970e04dd6b5..575c56cdd7c712 100644 --- 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 index efc28731bd0c36..77dc4804c1f986 100644 --- 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/);