lib: refactor stream_wrap #16158

addaleax · 2017-10-12T06:02:03Z

Another first step into making parts of the code base more accessible.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

lib/

fhinkel · 2017-10-12T12:53:29Z

lib/net.js

@@ -248,7 +248,7 @@ function Socket(options) {
      this._handle.reading = false;
      this._handle.readStop();
      this._readableState.flowing = false;
-    } else {
+    } else if (!options.manualStart) {
      this.read(0);
    }


Add an else { error...} case here?

Never mind, this is not an exhaustive if else.

fhinkel · 2017-10-12T12:54:59Z

lib/internal/wrap_js_stream.js

+const errors = require('internal/errors');
+
+/* This class serves as a wrapper for when the C++ side of Node wants access
+ * to a standard JS stream. For example, TLS or HTTP do not operate on network


on a network resource or on network resources.

fhinkel · 2017-10-12T12:56:13Z

lib/internal/wrap_js_stream.js

+ * For the common case, i.e. a TLS socket wrapping around a net.Socket, we
+ * can skip going through the JS layer and let TLS access the raw C++ handle
+ * of a net.Socket. The flipside of this is that, to maintain composability,
+ * a way to create "fake" net.Socket instances that call back into a


Nit: change to active voice:
The flipside of this is that, to maintain composability, we need a way to create ....

jasnell · 2017-10-12T12:56:24Z

lib/internal/wrap_js_stream.js

+ * a way to create "fake" net.Socket instances that call back into a
+ * "real" JavaScript stream is needed. JSStreamWrap is exactly this.
+ */
+class JSStreamWrap extends Socket {


We need to be certain that the change to the ES6 class is not going to break the planet, here.

I think we are, there is no constructor without new in the original one.

Yes. Also, this class exposes only limited functionality – you effectively can’t use it for any other purpose than what Node core does.

Ok. I can't come up with anything specific that would break so I'm ++

jasnell · 2017-10-12T12:57:43Z

lib/internal/wrap_js_stream.js

+      debug('close');
+      this.doClose(cb);
+    };
+    handle.isAlive = () => this.isAlive();


do these really need to be closures?

They were before. Can we defer optimizing to later work?

fhinkel · 2017-10-12T13:00:18Z

lib/internal/wrap_js_stream.js

+    const ondata = (chunk) => {
+      if (typeof chunk === 'string' ||
+          stream._readableState.objectMode === true) {
+        // Make sure that no further `data` events will happen


Super-Nit: period at the end of the comment.

jasnell · 2017-10-12T13:09:41Z

Marking semver-major defensively. This is a change to an _-prefixed file, but it's one that has been around for quite some time. @ChALkeR ...would it be possible to get some insights into what (if any) userrland modules are using this that could be impacted?

mcollina

Very good work! I think we can take the refactoring a bit further and avoid some closure allocations.

mcollina · 2017-10-12T13:26:47Z

lib/_stream_wrap.js

-    cb();
-  });
-};
+module.exports = require('internal/wrap_js_stream');


I would deprecate this file, people should not be requiring this in any form.

If we go with semver-major, yes.

mcollina · 2017-10-12T13:29:37Z

lib/internal/wrap_js_stream.js

+ * a way to create "fake" net.Socket instances that call back into a
+ * "real" JavaScript stream is needed. JSStreamWrap is exactly this.
+ */
+class JSStreamWrap extends Socket {


I think we are, there is no constructor without new in the original one.

mcollina · 2017-10-12T13:30:11Z

lib/internal/wrap_js_stream.js

+    handle.onreadstart = () => this.readStart();
+    handle.onreadstop = () => this.readStop();
+    handle.onshutdown = (req) => this.doShutdown(req);
+    handle.onwrite = (req, bufs) => this.doWrite(req, bufs);


some of those would be better of to be bind(). Now it's faster than allocating a closure.

mcollina · 2017-10-12T13:31:05Z

lib/internal/wrap_js_stream.js

+      debug('data', chunk.length);
+      if (this._handle)
+        this._handle.readBuffer(chunk);
+    };


I would prefer if we could optimize this function so that it's not a closure anymore, but a top level function.

mcollina · 2017-10-12T13:34:36Z

lib/internal/wrap_js_stream.js

+        handle.doAfterWrite(req);
+        handle.finishWrite(req, errCode);
+      });
+    }


can we avoid allocating two closures for every write? I know it would make the code uglier, but it's probably the right moment to do.

The innermost can be replaced by a top-level function with handle, item, req and errCode as parameters. The top-level one can be allocated in the constructor, and we can store pending as an object property (maybe behind a symbol).

mcollina · 2017-10-12T13:35:36Z

lib/internal/wrap_js_stream.js

+      assert.strictEqual(this._handle, null);
+      cb();
+    });
+  }


this function should really be top-level. The while loop can really get cpu-intensive.

addaleax · 2017-10-12T15:14:51Z

Marking semver-major defensively. This is a change to an _-prefixed file, but it's one that has been around for quite some time. @ChALkeR ...would it be possible to get some insights into what (if any) userrland modules are using this that could be impacted?

I would be extremely surprised if something comes up. Are you marking this semver-major out of principle or because you can actually imagine a use case somebody is having?

(If somebody uses this, it’s probably @indutny because there’s basically nobody else around who knows what it does…)

addaleax · 2017-10-12T15:16:01Z

@mcollina We can talk about optimizations, but this isn’t creating any new closures – can we avoid nitpicking for an actual PR that does those optimizations? Also, to reiterate what the comment states: This is the slow path. Optimizing this doesn’t actually have much real-world impact.

jasnell · 2017-10-12T15:24:02Z

Marking it semver-major out of principle and an abundance of caution. I'm good with it landing as a patch, but we should do a small amount of due diligence first.

addaleax · 2017-10-12T15:25:12Z

Yeah, we can wait for an npm search for it. :)

mcollina · 2017-10-12T16:43:26Z

Currently git is not marking these files as moved, but rather as new files. If this is marked semver-major, this makes sense, and we can as well refactor and improve them.

I think we should land this as semver-major, and spend a little more time improving this.

I propose you keep the first commit in this PR. Once that is landed, we do another PR with the ES6 refactoring and we deprecate the old file.

jasnell · 2017-10-13T01:06:57Z

Definitely agree with @mcollina about wanting to avoid the closures if possible (looking them over, it should definitely be easily possible).

addaleax · 2017-10-13T02:14:50Z

I don’t disagree about avoiding closures in general, but I would really say that’s not the scope of this PR.

addaleax · 2017-10-13T02:16:09Z

Also, if this is semver-major, could you give a hypothetical example of code that might break?

jasnell · 2017-10-13T02:25:06Z

There likely isn't any, again it's only defensive. Now that I've had a moment free to do a search on github and Google and I'm not seeing anything of any concern. If we cannot really identify anything that would break, I'm good with dropping the major label.

I disagree on the closure bit but I can open a separate pr that handles that.

addaleax · 2017-10-13T02:27:06Z

I’ll at least try to get @ChALkeR’s tooling to run again, it might be nice anyway if somebody else had practice in that (and I know I got it to build at least once) :)

addaleax · 2017-10-17T22:37:14Z

CI: https://ci.nodejs.org/job/node-test-commit/13229/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1013/

This makes a subsequent possible deprecation easier. PR-URL: nodejs#16158 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

This makes a subsequent possible deprecation easier. PR-URL: nodejs#16158 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

PR-URL: nodejs#16158 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

This makes a subsequent possible deprecation easier. PR-URL: #16158 Backport-PR-URL: #16626 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

PR-URL: #16158 Backport-PR-URL: #16626 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

This makes a subsequent possible deprecation easier. PR-URL: #16158 Backport-PR-URL: #16626 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

PR-URL: #16158 Backport-PR-URL: #16626 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

MylesBorins · 2017-11-14T16:12:32Z

@addaleax I've added the don't land label for 6.x... lmk if we should change that

This makes a subsequent possible deprecation easier. PR-URL: nodejs/node#16158 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

PR-URL: nodejs/node#16158 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

Its unused by node, and doesn't have a reasonable use outside of node. See: nodejs#25153 See: nodejs#16158

Its unused by node, and doesn't have a reasonable use outside of node. See: #25153 See: #16158 PR-URL: #26245 Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

nodejs-github-bot added build Issues and PRs related to build files or the CI. net Issues and PRs related to the net subsystem. stream Issues and PRs related to the stream subsystem. labels Oct 12, 2017

addaleax changed the title ~~lib: reactor stream_wrap~~ lib: refactor stream_wrap Oct 12, 2017

fhinkel reviewed Oct 12, 2017

View reviewed changes

jasnell reviewed Oct 12, 2017

View reviewed changes

jasnell requested a review from mcollina October 12, 2017 12:58

fhinkel reviewed Oct 12, 2017

View reviewed changes

fhinkel approved these changes Oct 12, 2017

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 12, 2017

mcollina reviewed Oct 12, 2017

View reviewed changes

cjihrig approved these changes Oct 16, 2017

View reviewed changes

addaleax force-pushed the refactor-js-stream branch from 8fc47f2 to 4cd793f Compare October 17, 2017 20:35

addaleax removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 17, 2017

lib: move _stream_wrap into internals

beb69df

This makes a subsequent possible deprecation easier. PR-URL: nodejs#16158 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]>

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

addaleax mentioned this pull request Oct 30, 2017

wrap_js_stream: adding test for readStop #16356

Closed

2 tasks

gibfahn mentioned this pull request Oct 31, 2017

v8.9.0 proposal #16630

Merged

MylesBorins added the dont-land-on-v6.x label Nov 14, 2017

addaleax added backported-to-v8.x and removed backport-requested-v8.x labels Jan 10, 2018

sam-github mentioned this pull request Feb 21, 2019

lib: deprecate _stream_wrap #26245

Closed

3 tasks

sam-github added a commit to sam-github/node that referenced this pull request Mar 2, 2019

lib: deprecate _stream_wrap

a1fb336

Its unused by node, and doesn't have a reasonable use outside of node. See: nodejs#25153 See: nodejs#16158

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: refactor stream_wrap #16158

lib: refactor stream_wrap #16158

addaleax commented Oct 12, 2017

fhinkel Oct 12, 2017

fhinkel Oct 12, 2017

fhinkel Oct 12, 2017

fhinkel Oct 12, 2017

jasnell Oct 12, 2017

mcollina Oct 12, 2017

addaleax Oct 12, 2017

jasnell Oct 18, 2017

jasnell Oct 12, 2017

addaleax Oct 12, 2017

fhinkel Oct 12, 2017

jasnell commented Oct 12, 2017

mcollina left a comment

mcollina Oct 12, 2017

addaleax Oct 12, 2017

mcollina Oct 12, 2017

mcollina Oct 12, 2017

mcollina Oct 12, 2017

mcollina Oct 12, 2017

mcollina Oct 12, 2017

addaleax commented Oct 12, 2017

addaleax commented Oct 12, 2017 •

edited

Loading

jasnell commented Oct 12, 2017

addaleax commented Oct 12, 2017

mcollina commented Oct 12, 2017

jasnell commented Oct 13, 2017

addaleax commented Oct 13, 2017

addaleax commented Oct 13, 2017

jasnell commented Oct 13, 2017

addaleax commented Oct 13, 2017

addaleax commented Oct 17, 2017

MylesBorins commented Nov 14, 2017

lib: refactor stream_wrap #16158

lib: refactor stream_wrap #16158

Conversation

addaleax commented Oct 12, 2017

Checklist

Affected core subsystem(s)

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

jasnell commented Oct 12, 2017

mcollina 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

addaleax commented Oct 12, 2017

addaleax commented Oct 12, 2017 • edited Loading

jasnell commented Oct 12, 2017

addaleax commented Oct 12, 2017

mcollina commented Oct 12, 2017

jasnell commented Oct 13, 2017

addaleax commented Oct 13, 2017

addaleax commented Oct 13, 2017

jasnell commented Oct 13, 2017

addaleax commented Oct 13, 2017

addaleax commented Oct 17, 2017

MylesBorins commented Nov 14, 2017

addaleax commented Oct 12, 2017 •

edited

Loading