[x] http: added closed property by ronag · Pull Request #28621 · nodejs/node

ronag · 2019-07-10T11:14:17Z

This is an alternative to finished and onFinished.isFinished to more accurately indicate when a request/response has completed.

With this PR the frequently used on-finished npm package should no longer be necessary as the "same" (assumed) functionality can be achieved using the 'close' event and closed property.

Refs: #28412

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

BridgeAR · 2019-07-11T14:01:31Z

@nodejs/http PTAL

ronag · 2019-07-12T10:33:06Z

I've got a lot of other pending fixes and PR's that depend on this. Would be nice to get it merged before I start "spamming" with new PR's.

Trott · 2019-07-13T04:51:20Z

@nodejs/collaborators This could use some reviews.

sebdeckers

Nice, LGTM. Would be great if there were docs and a test for the added http2 compat properties.

benjamingr

The code changes look good but I am just not sure we should do this. It adds another property and I am really not sure why we need it vs. just listening to an event. An extra variable for every single http request for something I don't understand isn't something I'm comfortable approving. Can you motivate this?

ronag · 2019-07-13T11:21:36Z

@benjamingr Because currently, there is no way to check whether an http object is closed or not. If you are using a framework such as koa or similar it might already be too late to register a 'close' listener by the time you get access to the object. Also adding a close listener on every http request has a more significant performance overhead.

In summary there are cases already out in the wild which cannot solve this and are incorrectly depending on msg.finished or onFinished.isFinished non of which properly implement it.

ronag · 2019-07-13T11:27:48Z

Also I have other pending fixes that depend on this, e.g.

Every call after 'close' should be a noop. Currently we have that logic which depends on finished which is not enough.
a complete getter which fully replaces finished in terms of checking if any further useful work can be done.

ronag · 2019-07-13T11:42:12Z

If you are worried about size we can also do things like: nxtedition@7355716

benjamingr

I still think .closed should be done on the framework side but the argument regarding other fixes this would enable (in particular doing work on a closed request) are a compelling enough argument to me.

nodejs-github-bot · 2019-07-14T08:46:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/24425/

mcollina

I'm not convinced of this change. I would rather prefer that we switch IncomingMessage to use the the emitClose and autoDestroy option instead, making it less of a snowflake in the stream world. I'm not sure if that is actually doable.

ronag · 2019-07-14T16:28:38Z

@mcollina would it make more sense to rename closed to destroyed and that way it pretends to follows the streams spec. Later we can put extra work into removing this "hack" and use autoDestroy? i.e. from the outside it would be the "same" but the implementation detail is suboptimal.

ronag · 2019-07-14T17:12:38Z

@mcollina I tried the autoClose & autoDestroy. But it seems a bit too complicated for me...

mcollina

I don't see tests or docs for http2.

ronag · 2019-07-16T08:41:28Z

@mcollina you asked me to add more checks to https://github.com/nodejs/node/blob/master/test/parallel/test-http-connect-req-res.js.

Those checks will actually fail since e.g. req.'close' is emitted before `socket.'end'. But that bug is not related to this PR per se and is something I suggest we solve separately and regardless if this PR lands or not.

mcollina · 2019-07-16T08:47:11Z

Those checks will actually fail since e.g. req.'close' is emitted before `socket.'end'. But that bug is not related to this PR per se and is something I suggest we solve separately and regardless if this PR lands or not.

Please add tests that verifies the current behavior, having it unspecified is going to cause more issues. Add a comment on it that explain your thinking. I would also open a separate issue about it.

ronag · 2019-07-16T08:54:20Z

@mcollina: Done, if you agree, I will add a separate PR with test that are "correct" and fail.

mcollina · 2019-07-16T09:02:02Z


+ get closed() {
+ return this[kState].closed;
+ }


No tests have been added for these.

ronag · 2019-08-26T13:20:44Z

Closing for now.

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 10, 2019

ronag force-pushed the feat-http-closed branch 8 times, most recently from 847be88 to a554a24Compare July 10, 2019 11:40

vsemozhetbyt reviewed Jul 10, 2019
View reviewed changes

Comment threaddoc/api/http.md Outdated
Comment threaddoc/api/http.md Outdated
Comment threaddoc/api/http.md Outdated
Comment threaddoc/api/http.md Outdated

ronag force-pushed the feat-http-closed branch 4 times, most recently from d5bb19f to 02b82d8Compare July 10, 2019 14:31

ronag mentioned this pull request Jul 10, 2019
http: add complete property #28628
Closed
4 tasks

ronag force-pushed the feat-http-closed branch from 68eaf08 to 02b82d8Compare July 10, 2019 22:06

ronag force-pushed the feat-http-closed branch 2 times, most recently from 0321930 to 49ab8faCompare July 12, 2019 10:21

ronag mentioned this pull request Jul 12, 2019
response is only finished if socket is detached jshttp/on-finished#31
Open

sebdeckers reviewed Jul 13, 2019
View reviewed changes

benjamingr reviewed Jul 13, 2019
View reviewed changes

benjamingr approved these changes Jul 13, 2019
View reviewed changes

ronag mentioned this pull request Jul 13, 2019
http: don't error after http client response #28668
Closed
4 tasks

ronag force-pushed the feat-http-closed branch from 49ab8fa to 3f36defCompare July 13, 2019 12:52

ronag commented Jul 13, 2019
View reviewed changes

Comment threadlib/_http_server.js Outdated

ronag force-pushed the feat-http-closed branch 3 times, most recently from f8eeaee to 9bcc445Compare July 13, 2019 12:55

mcollina requested changes Jul 14, 2019
View reviewed changes

Comment threadlib/_http_client.js Outdated

ronag force-pushed the feat-http-closed branch from 9bcc445 to 88e06a3Compare July 14, 2019 11:27

mcollina requested changes Jul 16, 2019
View reviewed changes

Comment threadlib/_http_server.js Outdated
Comment threadlib/_http_server.js Outdated
Comment threadtest/parallel/test-http-connect-req-res.js Outdated
Comment threadtest/parallel/test-http-req-res-close.js Outdated

ronag force-pushed the feat-http-closed branch 2 times, most recently from 45eead6 to 2f5b684Compare July 16, 2019 08:38

http: added closed property
e4ea031

ronag force-pushed the feat-http-closed branch from 2f5b684 to e4ea031Compare July 16, 2019 08:54

mcollina requested changes Jul 16, 2019
View reviewed changes

Comment threadlib/internal/http2/compat.js

get closed() {
return this[kState].closed;
}
mcollinaJul 16, 2019
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No tests have been added for these.

ronag mentioned this pull request Jul 23, 2019
stream: eos on closed #28748
Closed
4 tasks

ronag closed this Aug 26, 2019

ronag changed the title ~~http: added closed property~~[x] http: added closed propertyAug 26, 2019

Uh oh!

Uh oh!

Conversation

ronag commented Jul 10, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR commented Jul 11, 2019

Uh oh!

ronag commented Jul 12, 2019

Uh oh!

Trott commented Jul 13, 2019

Uh oh!

sebdeckers left a comment

Choose a reason for hiding this comment

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

ronag commented Jul 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 13, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 13, 2019

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 14, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronag commented Jul 14, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronag commented Jul 14, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ronag commented Jul 16, 2019

Uh oh!

mcollina commented Jul 16, 2019

Uh oh!

ronag commented Jul 16, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcollinaJul 16, 2019

Choose a reason for hiding this comment

Uh oh!

ronag commented Aug 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

ronag commented Jul 10, 2019•
edited
Loading

ronag commented Jul 13, 2019•
edited
Loading

ronag commented Jul 13, 2019•
edited
Loading

ronag commented Jul 14, 2019•
edited
Loading

ronag commented Jul 16, 2019•
edited
Loading