http: clean up dead code and unused properties #1572

mscdex · 2015-05-01T06:17:43Z

No description provided.

mscdex · 2015-05-01T06:19:20Z

lib/_http_common.js

-      stream.push(null);
-  }
-
-  if (stream && !parser.incoming._pendings.length) {


This is the only place I could find where ._pendings was actually used, so since nothing was ever added to the array, the second part of the conditional was always evaluating to true. Thus, the stream.push(null) can be merged up above.

chrisdickinson · 2015-05-01T07:07:05Z

lib/_http_incoming.js

@@ -49,7 +47,6 @@ function IncomingMessage(socket) {
  // response (client) only
  this.statusCode = null;
  this.statusMessage = null;
-  this.client = this.socket;


I'd rather we deprecate this property than delete it outright.

chrisdickinson · 2015-05-01T07:11:54Z

Other than the deprecation preference, LGTM. _pendings appears to have been introduced by e6b6075, and superceded by streams2.

mscdex · 2015-05-01T07:40:29Z

@chrisdickinson I added a deprecation notice for .client. Does that look alright?

chrisdickinson · 2015-05-01T07:59:39Z

lib/_http_incoming.js

@@ -63,6 +60,9 @@ util.inherits(IncomingMessage, Stream.Readable);

 exports.IncomingMessage = IncomingMessage;

+IncomingMessage.prototype.__defineGetter__('client', util.deprecate(function() {


I think we prefer Object.defineProperty (like so, with a util.deprecate'd getter function as you have here), but otherwise this looks good!

bnoordhuis · 2015-05-01T09:52:29Z

lib/_http_outgoing.js

@@ -613,8 +611,7 @@ OutgoingMessage.prototype._flush = function() {
      var outputEncodings = this.outputEncodings;
      var outputCallbacks = this.outputCallbacks;
      for (var i = 0; i < outputLength; i++) {
-        ret = socket.write(output[i], outputEncodings[i],
-                           outputCallbacks[i]);
+        ret = socket.write(output[i], outputEncodings[i], outputCallbacks[i]);


Can you undo the style changes in this file?

bnoordhuis · 2015-05-01T09:52:54Z

LGTM with a request.

Fishrock123 · 2015-05-01T13:45:54Z

lib/_http_incoming.js

+  get: util.deprecate(function() {
+    return this.socket;
+  }, 'client is deprecated, use socket or connection instead')
+});


might still want to support set here also if this is not semver-major

What would the setter do? Throw a deprecation error? Set this.socket? Set this.client again? Something else?

would set this.socket. Could also throw a dep error I suppose.

chrisdickinson · 2015-05-01T17:35:08Z

lib/_http_incoming.js

+  }, 'client is deprecated, use socket or connection instead'),
+  set: util.deprecate(function(val) {
+    this.socket = this.connection = val;
+  }, 'client is deprecated, use socket or connection instead')


Sorry, just saw this. We should use set to change this.client to whatever the passed value is, while not affecting socket.

(As written this changes behavior – previously setting .client would not affect .socket.)

ah, so it should initially be set to this.socket then?

Wait, if the setter is changing this.client, wouldn't we have to change the underlying property name (e.g. to something like this._client) otherwise it would lead to some kind of loop (setting this.client inside a this.client setter)?

Fishrock123 · 2015-05-07T14:00:35Z

lib/_http_incoming.js

+  set: util.deprecate(function(val) {
+    this._client = val;
+  }, 'client is deprecated, use socket or connection instead')
+});


Of course, this will break if anyone is trying to do delete req.client for whatever reason..

Fishrock123 · 2015-05-07T14:01:20Z

LGTM, @chrisdickinson?

brendanashworth · 2015-05-24T18:59:14Z

I think @chrisdickinson already signed off .

PR-URL: #1572 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

mscdex · 2015-05-25T05:10:49Z

Landed in 1eec5f0.

othiym23 · 2015-05-31T16:45:36Z

I'd like to state for the record that I'm pretty annoyed that this landed in a non-semver major version of io.js, as it is, as we have seen in #1808 with request (and transitively npm), a breaking change. Whether or not properties of objects created within Node are considered part of the exported API or not, they are, and to change them or move them around is always at least a semver-major change, and should be trailed by a proper deprecation cycle, starting with notes in the documentation before any changes are made to the code.

As @ceejbot says here and here:

Properties on Node-created objects are part of the exported interface.

If you prefix them with _ and people use them, they get what they deserve. But if you didn’t, you’re stuck with them in perpetuity, sorry.

Just because client isn't used within the Node codebase doesn't mean it's not used in the ecosystem, and we don't get to decide how these things are consumed. It's hard to predict things like people using .hasOwnProperty(), so that part isn't so bad. But "cleanup" like this doesn't belong in minor or patch releases.

Fishrock123 · 2015-05-31T16:59:28Z

and should be trailed by a proper deprecation cycle, starting with notes in the documentation before any changes are made to the code.

I'm not up to date with #1704, but that contains the WIP deprecation policy, which you might be interested in.

http.IncomingRequest.client isn't documented anyways, and I doubt we are going to document a deprecation on something that isn't documented in the first place. A warning should do I think, and it shouldn't effect anything.

it's hard to predict things like people using .hasOwnProperty()

The core problem here was oversight of this point, not what you are making it to be.

But "cleanup" like this doesn't belong in minor or patch releases.

It landed in a minor, so far deprecations have been fine to land in minors?

If you prefix them with _ and people use them, they get what they deserve. But if you didn’t, you’re stuck with them in perpetuity, sorry.

This isn't actually correct. For example, there is no way we could possibly rename or change _headers. Way too much code depends on it. Again, I suggest discussing the deprecation policy.

In the mean time, we're tying to fix the actual oversight of .hasOwnProperty(): #1852

othiym23 · 2015-05-31T17:43:14Z

http.IncomingRequest.client isn't documented anyways

As I discuss below, its lack of documentation is immaterial to this discussion, and is also probably a bug in the documentation, strictly speaking.

and I doubt we are going to document a deprecation on something that isn't documented in the first place. A warning should do I think, and it shouldn't effect anything.

That seems to directly contradict the actual experience we're having right now.

But "cleanup" like this doesn't belong in minor or patch releases.

It landed in a minor, so far deprecations have been fine to land in minors?

I'm talking about the removal of client in the first place. That's a semver-major change, as client is part of the exported interface of the object, even if it's undocumented. I learn this with nearly every release of npm where we change some observable bit of npm's behavior: you can't predict what the community is going to be relying upon.

If you prefix them with _ and people use them, they get what they deserve. But if you didn’t, you’re stuck with them in perpetuity, sorry.

This isn't actually correct. For example, there is no way we could possibly rename or change _headers. Way too much code depends on it.

That's the converse of the quoted section's point – the important thing here is that properties are a part of the object's interface, documented or not. Expectations are higher on platforms than they are on tooling, and they're higher on tooling than they are on simple libraries or applications.

Fishrock123 · 2015-05-31T17:51:19Z

That seems to directly contradict the actual experience we're having right now.

Right, because we did it incorrect on a technical level and overlooked hasOwnProperty()

I'm talking about the removal of client in the first place.

I'm not really sure I'd consider replacing something with deprecation getter/setters as a "removal", but we still do have to be careful yes. However, as @mscdex mentioned to me in irc, doing things that don't work with getters, like delete client, in this case would have broken things beforehand anyways.

That's the converse of the quoted section's point – the important thing here is that properties are a part of the object's interface, documented or not.

Right, because I agree with the former, but _properties actually also are.

ChALkeR · 2015-05-31T18:12:11Z

@Fishrock123 Deleting properties of the core objects was never supported and should never be used, it's an undefined behaviour. For example, delete res.socket will break things badly.

ceejbot · 2015-05-31T18:30:46Z

Renaming properties on objects created by Node core modules should be considered a breaking change, whether prefixed with the conventional _ or not. The humble _ is a contract with the developer that says "here be dragons", so removing those is more defensible. It would not be prudent for node core to do it, however, particularly for things that have been around a long time.

The right fix is a process & tooling fix, so humans are not continually burdened by having to deal with this & think about the ramifications of every change. Part of the release acceptance should be a tool that runs npm install && npm test on a representative sample of, say, 20 modules that would make the ecosystem flip if they broke. Isaac talks about this in this comment on stability gates. (While getting the npm tests themselves to pass as part of this would be great, here the basic smoke test of npm install with a cold cache would have failed, if I understand correctly.)

I would be willing to help make this so.

ChALkeR · 2015-05-31T18:45:13Z

@ceejbot I always thought that _-prefixed properties should be considered as «private», and that any changes to them would not be a semver-major. Except for the cases when it is known in advance that that will break things.

Fishrock123 · 2015-06-01T00:42:40Z

I would be willing to help make this so.

@ceejbot I've thought about having a suite of userland modules to test (As Rust is recently doing), however, to fully cover it there are two important points:

Also testing dependancies (easier)
Knowing how to run the tests for arbitrary modules (hard?)

Is there any way around the latter? I think we have access to more resources now to set something up like this, but I doubt I'll have time to work anything out in the next 2? weeks.

chrisdickinson · 2015-06-01T00:44:39Z

Build is working on npm package smoke tests over here: nodejs/build#82

mscdex · 2015-06-01T00:58:46Z

@Fishrock123 Shouldn't running tests be as simple as npm test?

ceejbot · 2015-06-01T01:02:25Z

npm install & npm test should be enough to run the tests on any module you select as suitable for this project.

PR-URL: nodejs/node#1572 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

mscdex reviewed May 1, 2015
View reviewed changes

mscdex added the http Issues or PRs related to the http subsystem. label May 1, 2015

chrisdickinson reviewed May 1, 2015
View reviewed changes

mscdex force-pushed the http-cleanup branch from f690ee2 to 1e5a5aa Compare May 1, 2015 07:38

chrisdickinson reviewed May 1, 2015
View reviewed changes

mscdex force-pushed the http-cleanup branch from 1e5a5aa to 81fd842 Compare May 1, 2015 08:17

bnoordhuis reviewed May 1, 2015
View reviewed changes

Fishrock123 reviewed May 1, 2015
View reviewed changes

mscdex force-pushed the http-cleanup branch 2 times, most recently from 7fee207 to ebe5d52 Compare May 1, 2015 15:58

chrisdickinson reviewed May 1, 2015
View reviewed changes

http: simplify code and remove unused properties

f5615d7

mscdex force-pushed the http-cleanup branch from ebe5d52 to f5615d7 Compare May 2, 2015 01:37

Fishrock123 reviewed May 7, 2015
View reviewed changes

mscdex closed this May 25, 2015

mscdex deleted the http-cleanup branch May 25, 2015 05:11

This was referenced May 27, 2015

Release proposal: v3.0.0 #1807

Closed

Release proposal: v2.2.0 #1808

Merged

ChALkeR mentioned this pull request May 31, 2015

Replace '.client' with '.socket' as the former was deprecated in 2.2.0. request/request#1615

Merged

othiym23 mentioned this pull request May 31, 2015

Error code: ESSL when trying to install npm/npm#8406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: clean up dead code and unused properties #1572

http: clean up dead code and unused properties #1572

mscdex commented May 1, 2015

mscdex May 1, 2015

chrisdickinson May 1, 2015

chrisdickinson commented May 1, 2015

mscdex commented May 1, 2015

chrisdickinson May 1, 2015

bnoordhuis May 1, 2015

bnoordhuis commented May 1, 2015

Fishrock123 May 1, 2015

mscdex May 1, 2015

Fishrock123 May 1, 2015

chrisdickinson May 1, 2015

chrisdickinson May 1, 2015

Fishrock123 May 1, 2015

mscdex May 1, 2015

Fishrock123 May 7, 2015

Fishrock123 commented May 7, 2015

brendanashworth commented May 24, 2015

mscdex commented May 25, 2015

othiym23 commented May 31, 2015

Fishrock123 commented May 31, 2015

othiym23 commented May 31, 2015

Fishrock123 commented May 31, 2015

ChALkeR commented May 31, 2015

ceejbot commented May 31, 2015

ChALkeR commented May 31, 2015

Fishrock123 commented Jun 1, 2015

chrisdickinson commented Jun 1, 2015

mscdex commented Jun 1, 2015

ceejbot commented Jun 1, 2015

		@@ -63,6 +60,9 @@ util.inherits(IncomingMessage, Stream.Readable);

		exports.IncomingMessage = IncomingMessage;

		IncomingMessage.prototype.__defineGetter__('client', util.deprecate(function() {

http: clean up dead code and unused properties #1572

http: clean up dead code and unused properties #1572

Conversation

mscdex commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisdickinson commented May 1, 2015

mscdex commented May 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented May 1, 2015

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

Fishrock123 commented May 7, 2015

brendanashworth commented May 24, 2015

mscdex commented May 25, 2015

othiym23 commented May 31, 2015

Fishrock123 commented May 31, 2015

othiym23 commented May 31, 2015

Fishrock123 commented May 31, 2015

ChALkeR commented May 31, 2015

ceejbot commented May 31, 2015

ChALkeR commented May 31, 2015

Fishrock123 commented Jun 1, 2015

chrisdickinson commented Jun 1, 2015

mscdex commented Jun 1, 2015

ceejbot commented Jun 1, 2015