stream: throw invalid argument errors

Logic errors that do not depend on stream state should throw instead of invoke callback and emit error.
nodejs · Feb 17, 2020 · 722975a · 722975a
1 parent 4c746a6
commit 722975a
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 51 deletions.
diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js
@@ -273,13 +273,8 @@ Writable.prototype.write = function(chunk, encoding, cb) {
       cb = nop;
   }
 
-  let err;
-  if (state.ending) {
-    err = new ERR_STREAM_WRITE_AFTER_END();
-  } else if (state.destroyed) {
-    err = new ERR_STREAM_DESTROYED('write');
-  } else if (chunk === null) {
-    err = new ERR_STREAM_NULL_VALUES();
+  if (chunk === null) {
+    throw new ERR_STREAM_NULL_VALUES();
   } else if (!state.objectMode) {
     if (typeof chunk === 'string') {
       if (state.decodeStrings !== false) {
@@ -292,11 +287,18 @@ Writable.prototype.write = function(chunk, encoding, cb) {
       chunk = Stream._uint8ArrayToBuffer(chunk);
       encoding = 'buffer';
     } else {
-      err = new ERR_INVALID_ARG_TYPE(
+      throw new ERR_INVALID_ARG_TYPE(
         'chunk', ['string', 'Buffer', 'Uint8Array'], chunk);
     }
   }
 
+  let err;
+  if (state.ending) {
+    err = new ERR_STREAM_WRITE_AFTER_END();
+  } else if (state.destroyed) {
+    err = new ERR_STREAM_DESTROYED('write');
+  }
+
   if (err) {
     process.nextTick(cb, err);
     errorOrDestroy(this, err, true);

diff --git a/test/parallel/test-stream-writable-invalid-chunk.js b/test/parallel/test-stream-writable-invalid-chunk.js
@@ -2,20 +2,19 @@
 
 const common = require('../common');
 const stream = require('stream');
+const assert = require('assert');
 
 function testWriteType(val, objectMode, code) {
   const writable = new stream.Writable({
     objectMode,
     write: () => {}
   });
-  if (!code) {
-    writable.on('error', common.mustNotCall());
-  } else {
-    writable.on('error', common.expectsError({
-      code: code,
-    }));
+  writable.on('error', common.mustNotCall());
+  if (code) {
+    assert.throws(() => {
+      writable.write(val);
+    }, { code });
   }
-  writable.write(val);
 }
 
 testWriteType([], false, 'ERR_INVALID_ARG_TYPE');

diff --git a/test/parallel/test-stream-writable-null.js b/test/parallel/test-stream-writable-null.js
@@ -16,31 +16,22 @@ class MyWritable extends stream.Writable {
 
 {
   const m = new MyWritable({ objectMode: true });
-  m.write(null, (err) => assert.ok(err));
-  m.on('error', common.expectsError({
-    code: 'ERR_STREAM_NULL_VALUES',
-    name: 'TypeError',
-    message: 'May not write null values to stream'
-  }));
-}
-
-{ // Should not throw.
-  const m = new MyWritable({ objectMode: true }).on('error', assert);
-  m.write(null, assert);
+  m.on('error', common.mustNotCall());
+  assert.throws(() => {
+    m.write(null);
+  }, {
+    code: 'ERR_STREAM_NULL_VALUES'
+  });
 }
 
 {
   const m = new MyWritable();
-  m.write(false, (err) => assert.ok(err));
-  m.on('error', common.expectsError({
-    code: 'ERR_INVALID_ARG_TYPE',
-    name: 'TypeError'
-  }));
-}
-
-{ // Should not throw.
-  const m = new MyWritable().on('error', assert);
-  m.write(false, assert);
+  m.on('error', common.mustNotCall());
+  assert.throws(() => {
+    m.write(false);
+  }, {
+    code: 'ERR_INVALID_ARG_TYPE'
+  });
 }
 
 { // Should not throw.

diff --git a/test/parallel/test-stream-writable-write-error.js b/test/parallel/test-stream-writable-write-error.js
@@ -4,19 +4,27 @@ const assert = require('assert');
 
 const { Writable } = require('stream');
 
-function expectError(w, arg, code) {
-  let errorCalled = false;
-  let ticked = false;
-  w.write(arg, common.mustCall((err) => {
-    assert.strictEqual(ticked, true);
-    assert.strictEqual(errorCalled, false);
-    assert.strictEqual(err.code, code);
-  }));
-  ticked = true;
-  w.on('error', common.mustCall((err) => {
-    errorCalled = true;
-    assert.strictEqual(err.code, code);
-  }));
+function expectError(w, arg, code, sync) {
+  if (sync) {
+    if (code) {
+      assert.throws(() => w.write(arg), { code });
+    } else {
+      w.write(arg);
+    }
+  } else {
+    let errorCalled = false;
+    let ticked = false;
+    w.write(arg, common.mustCall((err) => {
+      assert.strictEqual(ticked, true);
+      assert.strictEqual(errorCalled, false);
+      assert.strictEqual(err.code, code);
+    }));
+    ticked = true;
+    w.on('error', common.mustCall((err) => {
+      errorCalled = true;
+      assert.strictEqual(err.code, code);
+    }));
+  }
 }
 
 function test(autoDestroy) {
@@ -43,15 +51,15 @@ function test(autoDestroy) {
       autoDestroy,
       _write() {}
     });
-    expectError(w, null, 'ERR_STREAM_NULL_VALUES');
+    expectError(w, null, 'ERR_STREAM_NULL_VALUES', true);
   }
 
   {
     const w = new Writable({
       autoDestroy,
       _write() {}
     });
-    expectError(w, {}, 'ERR_INVALID_ARG_TYPE');
+    expectError(w, {}, 'ERR_INVALID_ARG_TYPE', true);
   }
 }