stream: throw invalid argument errors

Logic errors that do not depend on stream state should throw instead of invoke callback and emit error. PR-URL: nodejs#31831 Refs: nodejs#31818 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]>
mmomtchev · Feb 25, 2020 · 1f20912 · 1f20912
1 parent 9403250
commit 1f20912
Show file tree

Hide file tree

Showing 10 changed files with 92 additions and 88 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-fs-write-stream.js b/test/parallel/test-fs-write-stream.js
@@ -56,13 +56,12 @@ tmpdir.refresh();
 // Throws if data is not of type Buffer.
 {
   const stream = fs.createWriteStream(file);
-  stream.on('error', common.expectsError({
+  stream.on('error', common.mustNotCall());
+  assert.throws(() => {
+    stream.write(42);
+  }, {
     code: 'ERR_INVALID_ARG_TYPE',
     name: 'TypeError'
-  }));
-  stream.write(42, null, common.expectsError({
-    code: 'ERR_INVALID_ARG_TYPE',
-    name: 'TypeError'
-  }));
+  });
   stream.destroy();
 }
diff --git a/test/parallel/test-net-socket-write-error.js b/test/parallel/test-net-socket-write-error.js
@@ -2,19 +2,19 @@
 
 const common = require('../common');
 const net = require('net');
+const assert = require('assert');
 
 const server = net.createServer().listen(0, connectToServer);
 
 function connectToServer() {
   const client = net.createConnection(this.address().port, () => {
-    client.write(1337, common.expectsError({
+    client.on('error', common.mustNotCall());
+    assert.throws(() => {
+      client.write(1337);
+    }, {
       code: 'ERR_INVALID_ARG_TYPE',
       name: 'TypeError'
-    }));
-    client.on('error', common.expectsError({
-      code: 'ERR_INVALID_ARG_TYPE',
-      name: 'TypeError'
-    }));
+    });
 
     client.destroy();
   })

diff --git a/test/parallel/test-net-write-arguments.js b/test/parallel/test-net-write-arguments.js
@@ -1,20 +1,18 @@
 'use strict';
 const common = require('../common');
 const net = require('net');
-
+const assert = require('assert');
 const socket = net.Stream({ highWaterMark: 0 });
 
 // Make sure that anything besides a buffer or a string throws.
-socket.write(null, common.expectsError({
+socket.on('error', common.mustNotCall());
+assert.throws(() => {
+  socket.write(null);
+}, {
   code: 'ERR_STREAM_NULL_VALUES',
   name: 'TypeError',
   message: 'May not write null values to stream'
-}));
-socket.on('error', common.expectsError({
-  code: 'ERR_STREAM_NULL_VALUES',
-  name: 'TypeError',
-  message: 'May not write null values to stream'
-}));
+});
 
 [
   true,
@@ -29,10 +27,12 @@ socket.on('error', common.expectsError({
 ].forEach((value) => {
   // We need to check the callback since 'error' will only
   // be emitted once per instance.
-  socket.write(value, common.expectsError({
+  assert.throws(() => {
+    socket.write(value);
+  }, {
     code: 'ERR_INVALID_ARG_TYPE',
     name: 'TypeError',
     message: 'The "chunk" argument must be of type string or an instance of ' +
               `Buffer or Uint8Array.${common.invalidArgTypeHelper(value)}`
-  }));
+  });
 });
diff --git a/test/parallel/test-stream-writable-invalid-chunk.js b/test/parallel/test-stream-writable-invalid-chunk.js
@@ -2,20 +2,21 @@
 
 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());
+  writable.on('error', common.mustNotCall());
+  if (code) {
+    assert.throws(() => {
+      writable.write(val);
+    }, { code });
   } else {
-    writable.on('error', common.expectsError({
-      code: code,
-    }));
+    writable.write(val);
   }
-  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);
   }
 }
 

diff --git a/test/parallel/test-stream2-writable.js b/test/parallel/test-stream2-writable.js
@@ -422,12 +422,12 @@ const helloWorldBuffer = Buffer.from('hello world');
 {
   // Verify that error is only emitted once when failing in write.
   const w = new W();
-  w.on('error', common.mustCall((err) => {
-    assert.strictEqual(w._writableState.errorEmitted, true);
-    assert.strictEqual(err.code, 'ERR_STREAM_NULL_VALUES');
-  }));
-  w.write(null);
-  w.destroy(new Error());
+  w.on('error', common.mustNotCall());
+  assert.throws(() => {
+    w.write(null);
+  }, {
+    code: 'ERR_STREAM_NULL_VALUES'
+  });
 }
 
 {

diff --git a/test/parallel/test-zlib-invalid-input.js b/test/parallel/test-zlib-invalid-input.js
@@ -43,10 +43,11 @@ const unzips = [
 ];
 
 nonStringInputs.forEach(common.mustCall((input) => {
-  // zlib.gunzip should not throw an error when called with bad input.
-  zlib.gunzip(input, (err, buffer) => {
-    // zlib.gunzip should pass the error to the callback.
-    assert.ok(err);
+  assert.throws(() => {
+    zlib.gunzip(input);
+  }, {
+    name: 'TypeError',
+    code: 'ERR_INVALID_ARG_TYPE'
   });
 }, nonStringInputs.length));
 

diff --git a/test/parallel/test-zlib-object-write.js b/test/parallel/test-zlib-object-write.js
@@ -1,12 +1,14 @@
 'use strict';
 
 const common = require('../common');
+const assert = require('assert');
 const { Gunzip } = require('zlib');
 
 const gunzip = new Gunzip({ objectMode: true });
-gunzip.write({}, common.expectsError({
-  name: 'TypeError'
-}));
-gunzip.on('error', common.expectsError({
-  name: 'TypeError'
-}));
+gunzip.on('error', common.mustNotCall());
+assert.throws(() => {
+  gunzip.write({});
+}, {
+  name: 'TypeError',
+  code: 'ERR_INVALID_ARG_TYPE'
+});