[x] http: added closed property #28621

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

doc/api/http.md

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.

lib/_http_server.js

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.

lib/_http_client.js

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.

lib/_http_server.js

test/parallel/test-http-connect-req-res.js

test/parallel/test-http-req-res-close.js

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

lib/internal/http2/compat.js

@@ -446,6 +450,10 @@ class Http2ServerResponse extends Stream {
    stream.on('timeout', onStreamTimeout(kResponse));
  }

+  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 a554a24 Compare July 10, 2019 11:40

vsemozhetbyt reviewed Jul 10, 2019

View reviewed changes

doc/api/http.md Outdated Show resolved Hide resolved

doc/api/http.md Outdated Show resolved Hide resolved

doc/api/http.md Outdated Show resolved Hide resolved

doc/api/http.md Outdated Show resolved Hide resolved

ronag force-pushed the feat-http-closed branch 4 times, most recently from d5bb19f to 02b82d8 Compare 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 02b82d8 Compare July 10, 2019 22:06

ronag force-pushed the feat-http-closed branch 2 times, most recently from 0321930 to 49ab8fa Compare 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 3f36def Compare July 13, 2019 12:52

ronag commented Jul 13, 2019

View reviewed changes

lib/_http_server.js Show resolved Hide resolved

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

mcollina requested changes Jul 14, 2019

View reviewed changes

lib/_http_client.js Outdated Show resolved Hide resolved

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

mcollina requested changes Jul 16, 2019

View reviewed changes

lib/_http_server.js Outdated Show resolved Hide resolved

lib/_http_server.js Outdated Show resolved Hide resolved

test/parallel/test-http-connect-req-res.js Show resolved Hide resolved

test/parallel/test-http-req-res-close.js Show resolved Hide resolved

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

http: added closed property

e4ea031

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

mcollina requested changes Jul 16, 2019

View reviewed changes

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 property Aug 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[x] http: added closed property #28621

[x] http: added closed property #28621

ronag commented Jul 10, 2019 •

edited

Loading

BridgeAR commented Jul 11, 2019

ronag commented Jul 12, 2019

Trott commented Jul 13, 2019

sebdeckers left a comment

benjamingr left a comment

ronag commented Jul 13, 2019 •

edited

Loading

ronag commented Jul 13, 2019 •

edited

Loading

ronag commented Jul 13, 2019

benjamingr left a comment

nodejs-github-bot commented Jul 14, 2019

mcollina left a comment

ronag commented Jul 14, 2019 •

edited

Loading

ronag commented Jul 14, 2019

mcollina left a comment

ronag commented Jul 16, 2019

mcollina commented Jul 16, 2019

ronag commented Jul 16, 2019 •

edited

Loading

mcollina Jul 16, 2019

ronag commented Aug 26, 2019

[x] http: added closed property #28621

[x] http: added closed property #28621

Conversation

ronag commented Jul 10, 2019 • edited Loading

Checklist

BridgeAR commented Jul 11, 2019

ronag commented Jul 12, 2019

Trott commented Jul 13, 2019

sebdeckers left a comment

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

ronag commented Jul 13, 2019 • edited Loading

ronag commented Jul 13, 2019 • edited Loading

ronag commented Jul 13, 2019

benjamingr left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Jul 14, 2019

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Jul 14, 2019 • edited Loading

ronag commented Jul 14, 2019

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Jul 16, 2019

mcollina commented Jul 16, 2019

ronag commented Jul 16, 2019 • edited Loading

mcollina Jul 16, 2019

Choose a reason for hiding this comment

ronag commented Aug 26, 2019

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