Always call callbacks in lib/_http_outgoing.js by laino · Pull Request #1143 · nodejs/node

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

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

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

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

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.

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

Uh oh!

Conversation

laino commented Mar 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laino commented Mar 14, 2015

Uh oh!

laino commented Mar 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laino commented Mar 15, 2015

Uh oh!

laino commented Mar 16, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendanashworth commented Mar 17, 2015

Uh oh!

laino commented Mar 17, 2015

Uh oh!

bnoordhuis commented Mar 17, 2015

Uh oh!

laino commented Mar 17, 2015

Uh oh!

laino commented Mar 17, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants