Merge http2 stream & net socket #19527

aks- · 2018-03-22T04:21:27Z

extract out codebase which is identical in both net.js and http2/core.js into new stream_base object.

this should conclude #19060

i'm working on another pr where we can standardize the property name between net.js and http2/core.js. and refactor it further.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mscdex · 2018-03-22T06:17:03Z

I think we should be avoiding adding new methods/properties, underscore-prefixed or not. At the very least we should be using Symbols to hide such implementation details.

Changes like this will also need to be benchmarked appropriately.

addaleax · 2018-03-22T11:15:40Z

lib/internal/streams/stream_base.js

+const StreamBaseMethods = {
+  createWriteWrap(req, streamId, handle, oncomplete) {
+    if (streamId)
+      req.stream = streamId;


I think this could remain in the HTTP/2 code, it’s specific to the protocol.

addaleax · 2018-03-22T11:27:16Z

lib/internal/streams/stream_base.js

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


Could you move this file into lib/internal/? The streams/ directory is currently used almost exclusively for the stream module itself, but this implements parts of network & HTTP/2 streams

aha i see, makes sense. doing it right now.

addaleax · 2018-03-22T11:28:00Z

lib/net.js

+  const handle = this._handle;
+  this.createWriteWrap(req, null, handle, afterWrite);
+  if (writev) ret = this.writevGeneric(req, data, cb);
+  else ret = this.writeGeneric(req, data, encoding, cb);


Style nit: Can you put the bodies on their own lines? That’s more common style inside Node core, I think

aks- · 2018-03-22T15:13:46Z

@mscdex updated code to use symbols, can you please take a look to see if it looks good?

mscdex · 2018-03-22T18:35:50Z

lib/internal/stream_base.js

+}
+
+const StreamBase = {
+  [kCreateWriteWrap](req, handle, oncomplete) {


I would call this something else, since it's not actually creating and returning a WriteWrap instance. Perhaps something like kSetupWriteWrap or similar?

mscdex · 2018-03-22T18:41:36Z

lib/internal/stream_base.js

+    if (err) {
+      return this.destroy(errnoException(err, 'write', error), cb);
+    }
+    return null;


This is unnecessary and can be removed.

mscdex · 2018-03-22T18:41:40Z

lib/internal/stream_base.js

+    if (err) {
+      return this.destroy(errnoException(err, 'write', error), cb);
+    }
+    return null;


This is unnecessary and can be removed.

lpinca · 2018-03-22T20:53:46Z

lib/net.js

@@ -307,6 +313,7 @@ function Socket(options) {
  this[BYTES_READ] = 0;
 }
 util.inherits(Socket, stream.Duplex);
+StreamBase.apply(Socket.prototype);


Can we get rid of apply and use Object.assign()? I find it cleaner.

Object.assign(Socket.prototype, StreamBase);

I also find the StreamBase name a bit odd for a set of mixins.

yea, i agree. i called it StreamBaseMethods prior to this... maybe StreamSharedMethods? and +1 on using Object.assign will do soon

aks- · 2018-03-23T04:56:40Z

@mscdex @lpinca changes applied, can you please check now?

mcollina

I would still prefer prefer if the methods were top-level functions rather than properties of the prototype. However I wouldn't put a -1 on that one.

I would also rename the file to lib/internal/stream_base_commons.js. Those are tightly couple to StreamBase, which is slightly different from require('stream').

lpinca · 2018-03-23T08:41:46Z

👍 to @mcollina's suggestion of using top-level functions. That would also allow us to not use symbols.

mscdex · 2018-03-23T09:08:49Z

I also prefer top-level functions, especially since some were already that way to begin with. My suggestion of Symbols was if we absolutely needed them as properties.

apapirovski

A good start but this needs some cleanup. I wholeheartedly agree with the suggestions to use top level functions.

apapirovski · 2018-03-23T09:16:06Z