Always call callbacks in lib/_http_outgoing.js #1143

laino · 2015-03-13T12:24:56Z

The changes will make sure that callbacks given to end() or write() will always be called, with or without an error depending on the circumstances.

A summary of the changes:

A few paths where the code would've just returned have been changed to also call the supplied callback.
Callbacks remaining in this.outputCallbacks will be called once the stream closes.
Tests have been updated accordingly to test for the changes.

brendanashworth · 2015-03-14T00:34:14Z

lib/_http_outgoing.js

@@ -70,6 +70,27 @@ function OutgoingMessage() {
  this._header = null;
  this._headers = null;
  this._headerNames = {};
+
+  var that = this;


usually, a variable name of self would be used in this instance

laino · 2015-03-14T12:31:44Z

I modified some code to make it adhere to the style used by io.js (proposed by @brendanashworth)

laino · 2015-03-14T12:38:06Z

Just found "make jslint", one more commit to make it pass that too.

bnoordhuis · 2015-03-14T15:30:56Z

lib/_http_outgoing.js


  if (this.finished) {
-    var err = new Error('write after end');
+    err = new Error('write after end');


It would be slightly more memory-efficient here and elsewhere to create the Error object inside the nextTick callback.

The error object is created outside because otherwise it wouldn't contain the correct stack trace.

I guess that's a fair point. What's the motivation for emitting the exception as an error instead of throwing it? Both are going to be disruptive.

As a small aside, I notice that OutgoingMessage#write() still returns true, indicating it's okay to call it again. That seems like a buglet to me.

I left all return values unchanged. Currently return values are pretty much only used as an indicator of whether
the write had to be buffered (false) or was written instantly (true).

The reason for not throwing at that point is that the user still has no reliable (documented!) way of checking the state of the stream beforehand. He would have to wrap every write in try catch...

That's another thing bugging me about the state of the current stream API.

But even having to check some field every time before I do a write... nope! I'd rather handle all errors at one point: in my callback.

laino · 2015-03-15T13:19:38Z

More micro-commits!

laino · 2015-03-16T17:46:46Z

So. Is this ready to get merged? Will it get merged?

If not I'm ready to make the necessary changes.

brendanashworth · 2015-03-17T04:48:21Z

lib/_http_outgoing.js

+
+    for (var i = 0; i < outputCallbacks.length; i++) {
+      var callback = outputCallbacks[i];
+      if (typeof callback === 'function')


is this check necessary? will a callback ever not be of type 'function'?

It could be null or undefined if the user didn't specify one...

So maybe just:

if (callback) callback(err);

brendanashworth · 2015-03-17T04:54:45Z

I added some comments. Overall, I like what you've done with the code, but changing the tests to make them pass (as they didn't before? is this the case?) worries me. I'm unsure about the semver impact of this.

laino · 2015-03-17T21:46:19Z

I'd call not emitting/calling the callback with an error a bug. So this is a bugfix?

I'm also confident that this will not break any existing code, as the callback is usually called when the write fails. This just fixes an edge case where it wasn't (which happens to be reproduced in the test).

bnoordhuis · 2015-03-17T22:10:57Z

This PR is at least semver-minor, maybe semver-major. The big change is that HEAD responses with a body are now an error instead of a debug warning. It's pretty common for people to lump HEAD and GET responses together, even express.js used to do it at one time (and maybe it still does.)

Looking at this PR again, it looks to me like a single commit that introduces two changes: one is that it forces callbacks to run always, the other is that it turns some warnings into errors. It's perhaps better to split it out so that the uncontroversial changes can at least go in.

laino · 2015-03-17T22:26:03Z

Mhh. Right. I forgot about that change while writing my response...

I'll make the debug message -> error a separate PR.

Edit: Which should get some discussion anyways. Should it even be an error or should the callback just be called indicating that the write succeeded? I think we can all agree that NOT
calling the callback at all is not a good idea, as it could create hard-to-find bugs for people who
want to execute further logic after they've written some data (who would expect HEAD requests to be
the culprit?). To me the choice looks pretty much like callback(error) or callback() + debug message.

I'm not sure what is be preferable, but the latter is a change that wouldn't break code which doesn't explicitly deal with HEAD requests (and is as such not a breaking change).

laino · 2015-03-17T22:50:38Z

Removed the controversial change from this PR

bnoordhuis · 2015-03-17T22:59:09Z

lib/_http_outgoing.js

  if (this.finished) {
+    if (data && data.length > 0) {
+      //If the user tried to write data, tell him it failed


Style nit: please put a space after the slashes and use punctuation. I've been told some people take offense at gendered pronouns in comments.

I'm not sure how I feel about turning write-after-end into an error. It's arguably the proper thing to do but I worry that it's going to break a lot of existing code.

brendanashworth · 2015-04-28T01:48:06Z

lib/_http_outgoing.js

+    self.outputEncodings = [];
+    self.outputCallbacks = [];
+
+    for (var i = 0; i < outputCallbacks.length; i++) {


Is there a reason you cache the variable and reset it rather than using it raw in the for loop and resetting it afterwards?

I worry the callback function over which we have no control might do something fishy and mess up our state while we're working with it. If such things are nothing we are supposed to be guarding against, then I'll change it of course.

brendanashworth · 2015-07-13T08:44:05Z

Hi @laino! I'd still like to see this merged in, but it needs updates based on the comments and should probably be resubmitted as a new PR to master. Are you still interested in updating it?

laino · 2015-07-15T07:49:34Z

Yes. I'll definitely do that sometime soon. Thanks for the reminder - I somehow forgot about this PR.

brendanashworth · 2015-07-16T21:38:34Z

Sweet! No problem. I'm going to close this for now and await a new PR to master. Thanks!

Always call callbacks in lib/_http_outgoing.js

2dd0bc2

Fishrock123 added the http Issues or PRs related to the http subsystem. label Mar 13, 2015

brendanashworth reviewed Mar 14, 2015
View reviewed changes

Make code adhere to io.js style

d733196

Make jslint pass

8f20ed6

bnoordhuis reviewed Mar 14, 2015
View reviewed changes

Nils Kuhnhenn added 2 commits March 15, 2015 14:18

Change code style in test

6d4bd06

Change code style in another test

e712b26

brendanashworth reviewed Mar 17, 2015
View reviewed changes

Revert to only a debug message for writes to HEAD responses

10d5da8

bnoordhuis reviewed Mar 17, 2015
View reviewed changes

chrisdickinson added the land-on-master label Apr 17, 2015

brendanashworth reviewed Apr 28, 2015
View reviewed changes

brendanashworth closed this Jul 16, 2015

laino mentioned this pull request Jul 24, 2015

Always call callbacks in outgoing streams #2240

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always call callbacks in lib/_http_outgoing.js #1143

Always call callbacks in lib/_http_outgoing.js #1143

laino commented Mar 13, 2015

brendanashworth Mar 14, 2015

laino commented Mar 14, 2015

laino commented Mar 14, 2015

bnoordhuis Mar 14, 2015

laino Mar 14, 2015

bnoordhuis Mar 15, 2015

laino Mar 15, 2015

laino commented Mar 15, 2015

laino commented Mar 16, 2015

brendanashworth Mar 17, 2015

laino Mar 17, 2015

tellnes Mar 28, 2015

brendanashworth commented Mar 17, 2015

laino commented Mar 17, 2015

bnoordhuis commented Mar 17, 2015

laino commented Mar 17, 2015

laino commented Mar 17, 2015

bnoordhuis Mar 17, 2015

brendanashworth Apr 28, 2015

laino Jul 15, 2015

brendanashworth commented Jul 13, 2015

laino commented Jul 15, 2015

brendanashworth commented Jul 16, 2015

Always call callbacks in lib/_http_outgoing.js #1143

Always call callbacks in lib/_http_outgoing.js #1143

Conversation

laino commented Mar 13, 2015

Choose a reason for hiding this comment

laino commented Mar 14, 2015

laino commented Mar 14, 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

laino commented Mar 15, 2015

laino commented Mar 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Mar 17, 2015

laino commented Mar 17, 2015

bnoordhuis commented Mar 17, 2015

laino commented Mar 17, 2015

laino commented Mar 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendanashworth commented Jul 13, 2015

laino commented Jul 15, 2015

brendanashworth commented Jul 16, 2015