Implement new writable stream #9

ivan-tymoshenko · 2018-10-26T14:31:59Z

No description provided.

lundibundi · 2018-10-27T11:49:02Z

lib/writable.js

+  }
+}
+
+inherits(Writable, EventEmitter);


class Writable extends EventEmitter at the declaration site?

lundibundi · 2018-10-27T11:50:29Z

lib/writable.js

-  this.encoding = encoding;
-};
+class WritableBuffer {
+  constructor(


Unneeded line break around argument.

lundibundi · 2018-10-27T11:51:05Z

lib/writable.js

+  constructor(
+    highWaterMark
+  ) {
+    this.pointer = 0;


lundibundi · 2018-10-27T11:55:43Z

lib/writable.js

+  }
+
+  add(chunk) {
+    const writeSize = this.buffer.length - this.pointer;


remainingSpace, spaceLeft, remainingSize maybe? As writeSize is chunk.length IMO.

lundibundi · 2018-10-27T12:01:56Z

lib/writable.js

+    highWaterMark = DEFAULT_HIGH_WATERMARK,
+    defaultEncoding = 'utf-8',
+    write = null,
+    writev = null


There arguments should be reversed, the defaults go last.

lundibundi · 2018-10-27T16:03:33Z

lib/writable.js

+      this.writableLength = 0;
+      this.doWrite(this.buffer.getData());
+    };
+    if (this._writev) this._writev(data, cb);


_writev is used to write an array of data at once, but you pass just one big buffer here IIUC. This won't give any benefits.

lundibundi · 2018-10-27T16:05:50Z

lib/writable.js

+        this.writing = false;
+        return;
+      }
+      this.writableLength = 0;


I think this length should be tracked in your WritableBuffer instance.

lundibundi · 2018-10-27T16:15:16Z

lib/writable.js

+    this.emit('close');
+
+    const err = new Error('[ERR_STREAM_DESTROYED]');
+    this.write = (...args) => {


Why don't you just set destroyed and check it in the write function, I don't think it's worth overriding a function for such a use case.

lundibundi · 2018-10-27T16:18:19Z

lib/writable.js

+    }
+
+    const err = new Error('[ERR_STREAM_WRITE_AFTER_END]');
+    this.write = (...args) => {


Same as the comment in destroy.

lundibundi · 2018-10-27T16:18:49Z

lib/writable.js

+      process.nextTick(cb, err);
+    };
+
+    this.emit('finish');


I think 'finish' should only be emitted after all of the data has been sent to the underlying system. (i.e. similar to https://nodejs.org/api/stream.html#stream_event_finish)

ivan-tymoshenko · 2018-10-28T17:14:45Z

@lundibundi please, reviewe after refactoring.

ivan-tymoshenko · 2018-11-01T11:41:07Z

@lundibundi please reviewe.

tshemsedinov · 2018-11-01T11:20:24Z

lib/writable.js

+  add(chunk) {
+    const remainingSize = this.buffer.length - this.offset;
+
+    if (chunk.length < this.remainingSize) {


lib/writable.js

tshemsedinov · 2018-11-01T11:27:51Z

lib/writable.js

+      const currentBufferChunk = chunk.slice(0, this.remainingSize);
+      this.writeChunk(currentBufferChunk);
+      this.queue.push(this.buffer);
+      this.buffer = Buffer.alloc(this.buffer.length);


this.buffer = Buffer.alloc(this.highWaterMark);

tshemsedinov · 2018-11-01T11:47:35Z

lib/writable.js

+    }
+
+    if (typeof cb !== 'function') {
+      cb = () => {};


Use common.safeCallback or common.once. But safeCallback takes array args so we may need safeFunction:

const safeFunction = fn => typeof fn === 'function' ? fn : emptiness;

Do you want to add a safeFunction to a stream library or to common?

Why does this assignment is even needed? Can't you just add
if (cb) ... on line 117 and later on in this function?
Also, IMO having empty callback here is harmful as

it will grow the callbacks array needlessly

you will have it scheduled on nextTick in case of error process.nextTick(cb, error);

it makes passing errors to the callback harder as you may accidentally pass an error to the empty function thus ignoring it.

lundibundi · 2018-11-01T19:20:10Z

lib/writable.js

+    } else {
+      this._write(data, cb);
+    }
+    callbacks.forEach(cb => cb());


This should be run in the cb as callback call means that the data has been written, not that it was passed to the underlying backend (flushed). (you can check out node docs or _stream_writable/doWrite & _stream_writable/onwrite).

lundibundi · 2018-11-01T19:45:47Z

Also, @bugagashenkj could you add some tests?

ivan-tymoshenko · 2018-11-05T09:45:56Z

@lundibundi please reviewe.

lundibundi · 2018-11-05T09:53:39Z

lib/writable.js

+
+  writeChunk(chunk, encoding) {
+    if (typeof chunk === 'string') {
+      this.buffer.write(chunk, this.offset, encoding);


I think this can write more than chunk.length due to the encoding. You have to use the return value of the write call.

lundibundi · 2018-11-05T09:57:07Z

lib/writable.js

+    this.buffer = Buffer.alloc(highWaterMark);
+  }
+
+  writeChunk(chunk, encoding) {


This should probably be hidden under Symbol.

lib/writable.js

lundibundi · 2018-11-05T10:04:34Z

lib/writable.js

+    this.defaultEncoding = encoding;
+  }
+
+  error(err, cb) {


Also, private?

lundibundi · 2018-11-05T10:07:05Z

lib/writable.js

+
+    if (this.destroyed) {
+      const err = new Error('[ERR_STREAM_DESTROYED]');
+      this.emit(err, cb);


Suggested change

this.emit(err, cb);

this.error(err, cb);

?

lib/writable.js

package.json

test/writable.js

lundibundi · 2018-11-05T10:14:25Z

test/writable.js

+metatests.testSync('Writable / destroy', test => {
+  const [stream, ] = createStream(5);
+
+  stream.on('error', err => { test.isError(err); });


You should use test.plan in this test.

lundibundi · 2018-11-05T12:20:55Z

lib/writable.js

+    this.buffer = Buffer.alloc(highWaterMark);
+  }
+
+  [writeChunkFuncName](chunk) {


Nit: I think writeChunkSymbol will be clearer.

lundibundi · 2018-11-05T12:23:46Z

lib/writable.js

+
+  uncork() {
+    if (this.corked) this.corked--;
+    if (!this.corked && this.writableLength) {


Suggested change

if (!this.corked && this.writableLength) {

if (this.corked === 0 && this.writableLength) {

Also, you should also add check to avoud this.corked become <0.

lundibundi · 2018-11-05T12:28:10Z

lib/writable.js

+    this[errorFuncName](err);
+  }
+
+  validChunk(chunk, cb) {


Also private?

lib/writable.js

lundibundi · 2018-11-05T12:30:49Z

lib/writable.js

+    };
+
+    if (this._writev) {
+      if (Array.isArray(data) && data.length > 1) this._writev(data, cb);


I don't think there is any merit in checking data.length > 1.

lundibundi

LGTM with a few nits.
Should we add/make benchmarks here or land and then work on those?
/cc @tshemsedinov

lundibundi · 2018-11-05T21:47:35Z

lib/writable.js

+    this.buffer = Buffer.alloc(highWaterMark);
+  }
+
+  [writeChunkSymbol](chunk) {


Nit: actually, you don't export this class, so there is no need ti hide it's methods behind the symbols, though that's up to you.

lundibundi · 2018-11-05T21:48:29Z

lib/writable.js

+    this.defaultEncoding = encoding;
+  }
+
+  [errorFuncName](err, cb) {


errorSymbol or errorFuncSymbol?

lundibundi · 2018-11-05T22:32:59Z

lib/writable.js

+    }
+
+    if (this.ended) {
+      const err = new Error('[ERR_STREAM_WRITE_AFTER_END]');


Forgot about errors.

Why do you have errors as strings? Those should be potentially be separate error classes and each have unique code.

Forgot about errors.

ivan-tymoshenko · 2018-11-12T09:26:25Z

@lundibundi please review.

lundibundi · 2018-11-12T14:50:05Z

lib/fs-writable.js

+  ERR_OUT_OF_RANGE
+} = require('./errors');
+
+const { FSReqCallback, writeBuffers } = process.binding('fs');


process.binding is deprecated in node 11 and will most likely be removed in node 12
nodejs/node#22478 and
https://github.com/nodejs/node/blob/master/doc/changelogs/CHANGELOG_V11.md#2018-10-23-version-1100-current-jasnell

General
Use of process.binding() has been deprecated. Userland code using process.binding() should re-evaluate that use and begin migrating. If there are no supported API alternatives, please open an issue in the Node.js GitHub repository so that a suitable alternative may be discussed.

Could you post the results of benchmarks with and without using those methods?

ivan-tymoshenko · 2018-11-13T15:50:02Z

 chunkLength     kind     rate confidence.interval
           8 metarhia 11606494           229144.66
           8     node  3491421            82066.44
          64 metarhia  6354815           288684.74
          64     node  3114953            98569.04
         128 metarhia  4313109           174982.62
         128     node  2867002            80028.80

https://gist.github.com/bugagashenkj/a0f7cd4929d45e0c03ee11e19b3a87cf

ivan-tymoshenko · 2018-11-13T15:54:30Z

@lundibundi please review.

lib/fs-writable.js

lib/writable.js

ivan-tymoshenko · 2018-11-22T16:21:18Z

@lundibundi

lib/fs-writable.js

lib/writable.js

package.json

test/fs-writable.js

lundibundi · 2018-12-09T08:36:37Z

lib/fs-writable.js

+    }
+
+    fs.write(this.fd, data, 0, data.length, this.pos, (error, bytes) => {
+      if (error) this[errorSymbol](error, cb);


Missed return?

lundibundi · 2018-12-09T08:38:06Z

lib/fs-writable.js

+      er = er || err;
+      cb(er);
+      this.closed = true;
+      if (!er) this.emit('close');


What's the reason behind not emitting 'close' here but still emitting it in case of error in [errorSymbol]?

lundibundi · 2018-12-09T08:47:00Z

lib/fs-writable.js

+      this.emit('error', error);
+      this.emit('close');
+    }
+    if (cb) cb(error);


Now that I think about it, the cb should be callback for an operation that 'erroed' so IMO it should be called at the same time or before 'error' event.
SO this should probably be

if (cb) cb(error); if (this.autoClose) { this.destroy(error); } else { this.emit('error', error); this.emit('close'); }

lundibundi · 2018-12-09T08:51:48Z

lib/fs-writable.js

+    });
+  }
+
+  [closeSymbol](cb) {


Is this method unused?

lib/fs-writable.js

lundibundi · 2018-12-17T13:42:02Z

lib/writable.js

+
+  uncork() {
+    if (this.corked > 0) this.corked--;
+    if (this.corked === 0 && this.writableLength) {


I'd prefer to be explicit

Suggested change

if (this.corked === 0 && this.writableLength) {

if (this.corked === 0 && this.writableLength > 0) {

PR-URL: #9

tshemsedinov

I'd prefer callback instead of cb in most cases except those where we have two callbacks in enclosed contexts

tshemsedinov · 2019-01-10T13:52:42Z

lib/errors.js

@@ -0,0 +1,68 @@
+'use strict';
+
+class ERR_INVALID_ARG_TYPE extends TypeError {


I'd prefer UpperCamelCase InvalidArgTypeError

tshemsedinov · 2019-01-10T13:57:29Z

lib/writable.js

+    }
+    encoding = encoding.toLowerCase();
+    if (!Buffer.isEncoding(encoding))
+      throw new ERR_UNKNOWN_ENCODING(encoding);


if (!Buffer.isEncoding(encoding)) throw new ERR_UNKNOWN_ENCODING(encoding); }

tshemsedinov · 2019-01-10T13:59:21Z

lib/writable.js

+        if (this.ending) {
+          this.ended = true;
+          if (this._final) {
+            this._final(() => this.emit('finish'));


Suggested change

this._final(() => this.emit('finish'));

this._final(() => {

this.emit('finish');

});

tshemsedinov · 2019-01-10T14:00:41Z

lib/writable.js

+    this.callbacks = [];
+
+    const cb = () => {
+      callbacks.forEach(cb => cb());


Suggested change

callbacks.forEach(cb => cb());

callbacks.forEach(callback => {

callback();

});

tshemsedinov · 2019-01-10T14:01:34Z

lib/writable.js

+    return this.buffer.writableLength;
+  }
+
+  _destroy(err, cb) { cb(err); }


Suggested change

_destroy(err, cb) { cb(err); }

_destroy(err, cb) {

cb(err);

}

tshemsedinov · 2019-01-10T14:01:59Z

metastreams.js

-});
-
-module.exports = submodules;
+module.exports = Object.assign({}, ...submodules);


Suggested change

module.exports = Object.assign({}, ...submodules);

module.exports = { ...submodules };

I think these two implementations are doing different things.

tshemsedinov · 2019-01-10T14:02:48Z

package.json

@@ -25,9 +25,6 @@
  "engines": {
    "node": ">=6.0.0"


"node": ">=8.6.0"

In separate commit before this

PR-URL: #9

lundibundi · 2019-01-14T12:33:20Z

lib/fs-writable.js

+    fs.close(this.fd, (er) => {
+      er = er || err;
+      callback(er);
+      this.closed = true;


I think we should set this.closed = true; before calling a callback so that the callback will see a correct state.

PR-URL: #9

ivan-tymoshenko · 2019-01-22T15:22:56Z

/cc @lundibundi

PR-URL: #9

lundibundi · 2019-01-22T19:05:11Z

Landed in b5d195f and 1f4e659.

ivan-tymoshenko self-assigned this Oct 26, 2018

ivan-tymoshenko requested a review from lundibundi October 26, 2018 14:31

lundibundi reviewed Oct 27, 2018

View reviewed changes

ivan-tymoshenko force-pushed the new-writable-stream branch from 997934e to 1dc3ecc Compare November 1, 2018 11:36

tshemsedinov reviewed Nov 1, 2018

View reviewed changes

lundibundi reviewed Nov 1, 2018

View reviewed changes

ivan-tymoshenko force-pushed the new-writable-stream branch 3 times, most recently from 9207dfd to 2afbe12 Compare November 5, 2018 09:36

lundibundi reviewed Nov 5, 2018

View reviewed changes

ivan-tymoshenko force-pushed the new-writable-stream branch 2 times, most recently from d301605 to ca1dbd4 Compare November 5, 2018 12:03

lundibundi reviewed Nov 5, 2018

View reviewed changes

ivan-tymoshenko force-pushed the new-writable-stream branch from 824c0cc to 3505b5e Compare November 5, 2018 13:40

lundibundi previously approved these changes Nov 5, 2018

View reviewed changes

lundibundi reviewed Nov 5, 2018

View reviewed changes

ivan-tymoshenko force-pushed the new-writable-stream branch from f795dde to 245d50e Compare November 12, 2018 09:43

lundibundi reviewed Nov 12, 2018

View reviewed changes

ivan-tymoshenko force-pushed the new-writable-stream branch from 8669845 to 4862daa Compare November 14, 2018 08:25

ivan-tymoshenko requested review from nechaido and lundibundi November 15, 2018 11:52

nechaido reviewed Nov 19, 2018

View reviewed changes

lib/fs-writable.js Outdated Show resolved Hide resolved

lib/fs-writable.js Outdated Show resolved Hide resolved

lib/writable.js Outdated Show resolved Hide resolved

lib/writable.js Outdated Show resolved Hide resolved

ivan-tymoshenko requested a review from nechaido November 26, 2018 10:19

lundibundi reviewed Nov 26, 2018

View reviewed changes

ivan-tymoshenko requested a review from lundibundi November 27, 2018 13:51

lundibundi reviewed Dec 9, 2018

View reviewed changes

lundibundi reviewed Dec 10, 2018

View reviewed changes

lib/fs-writable.js Outdated Show resolved Hide resolved

ivan-tymoshenko requested a review from lundibundi December 11, 2018 12:57

nechaido approved these changes Dec 11, 2018

View reviewed changes

lundibundi approved these changes Dec 17, 2018

View reviewed changes

ivan-tymoshenko force-pushed the new-writable-stream branch from 112f36d to fa8c99d Compare December 18, 2018 08:20

ivan-tymoshenko added a commit that referenced this pull request Dec 18, 2018

Implement new writable stream

fa8c99d

PR-URL: #9

ivan-tymoshenko requested a review from tshemsedinov December 18, 2018 12:47

ivan-tymoshenko force-pushed the new-writable-stream branch from fa8c99d to 24597c9 Compare December 19, 2018 13:35

ivan-tymoshenko added a commit that referenced this pull request Dec 19, 2018

Implement new writable stream

24597c9

PR-URL: #9

tshemsedinov reviewed Jan 10, 2019

View reviewed changes

Update node version

c17ef66

ivan-tymoshenko force-pushed the new-writable-stream branch from 24597c9 to 3888e5c Compare January 11, 2019 14:06

ivan-tymoshenko added a commit that referenced this pull request Jan 11, 2019

Implement new writable stream

88a3df9

PR-URL: #9

ivan-tymoshenko requested a review from tshemsedinov January 11, 2019 14:37

lundibundi approved these changes Jan 14, 2019

View reviewed changes

Implement new writable stream

e598f03

PR-URL: #9

ivan-tymoshenko force-pushed the new-writable-stream branch from 3888e5c to e598f03 Compare January 14, 2019 12:50

tshemsedinov approved these changes Jan 22, 2019

View reviewed changes

lundibundi pushed a commit that referenced this pull request Jan 22, 2019

Update minimal node engine version to 8.6.0

b5d195f

PR-URL: #9

lundibundi pushed a commit that referenced this pull request Jan 22, 2019

Implement new writable stream

1f4e659

PR-URL: #9

lundibundi closed this Jan 22, 2019

lundibundi deleted the new-writable-stream branch January 22, 2019 19:05

	if (!this.corked && this.writableLength) {
	if (this.corked === 0 && this.writableLength) {

		@@ -0,0 +1,68 @@
		'use strict';

		class ERR_INVALID_ARG_TYPE extends TypeError {

-      callbacks.forEach(cb => cb());
+      callbacks.forEach(callback => {
+        callback();
+      });

	module.exports = Object.assign({}, ...submodules);
	module.exports = { ...submodules };

Implement new writable stream #9

Implement new writable stream #9

Conversation

ivan-tymoshenko commented Oct 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-tymoshenko commented Oct 28, 2018

ivan-tymoshenko commented Nov 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshemsedinov Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi Nov 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi commented Nov 1, 2018

ivan-tymoshenko commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lundibundi Nov 5, 2018 • edited Loading

Choose a reason for hiding this comment

ivan-tymoshenko commented Nov 12, 2018

Choose a reason for hiding this comment

ivan-tymoshenko commented Nov 13, 2018

ivan-tymoshenko commented Nov 13, 2018

ivan-tymoshenko commented Nov 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tshemsedinov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivan-tymoshenko commented Jan 22, 2019

lundibundi commented Jan 22, 2019

tshemsedinov Nov 1, 2018 •

edited

Loading

lundibundi Nov 1, 2018 •

edited

Loading

lundibundi Nov 5, 2018 •

edited

Loading

lundibundi Nov 5, 2018 •

edited

Loading

ivan-tymoshenko commented Nov 22, 2018 •

edited

Loading