http2: implement ref() and unref() on sessions #17620

kjin · 2017-12-11T23:13:39Z

This is my stab at adding ref and unref on ClientHttp2Session instances. It seems mostly straightforward, but there were a few questions I had in mind about this code change:

Should ClientHttp2Session instances be unrefd by default?
Should these be available on ServerHttp2Session objects as well? (my thought is no)
Should calling ref/unref be a no-op after the session has been destroyed, or could it return an error?

I saw this comment made by @addaleax about uv_prepare_t, but I don't really understand it. To me, this code change seems sufficient, but maybe I'm missing something.

Also, I updated the docs to separate ClientHttp2Session from Http2Session... which seemed to make sense considering that this distinction is present even in the codebase.

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

Affected core subsystem(s)

http2

addaleax

I saw this comment made by @addaleax about uv_prepare_t, but I don't really understand it. To me, this code change seems sufficient, but maybe I'm missing something.

The code has changed since then. :)

Should ClientHttp2Session instances be unrefd by default?

I don’t think so; if you have an active network connection, you usually want that to keep the event loop alive.

Should these be available on ServerHttp2Session objects as well? (my thought is no)

Why not? :)

Should calling ref/unref be a no-op after the session has been destroyed, or could it return an error?

It seems doing nothing is what net.Socket does, so I’d go with that? I think that’s what this code already does, right? 👍

addaleax · 2017-12-11T23:18:27Z

doc/api/http2.md

+
+#### clienthttp2session.unref()
+<!-- YAML
+added: ??? (TODO)


Can you use added: REPLACEME here? That way it gets picked up automatically by the release tooling :)

addaleax · 2017-12-11T23:22:23Z

lib/internal/http2/core.js

@@ -1086,6 +1086,18 @@ class ClientHttp2Session extends Http2Session {
    }
    return stream;
  }
+
+  ref() {
+    if (this[kSocket]) {


Can you also check whether typeof this[kSocket].ref === 'function'? HTTP/2 supports any stream as the underlying socket, so .ref might not be present at that point.

The test file that checks that is test/parallel/test-http2-generic-streams.js, if you want to extend that – I think calling .ref() there would crash right now?

kjin · 2017-12-12T01:57:38Z

Should these be available on ServerHttp2Session objects as well? (my thought is no)

Why not? :)

I thought http.Server instances didn't have them, but I now see that they inherit ref and unref from net.Server. I suppose I couldn't think of use cases for unrefing an http.Server earlier, but there's no reason now to not include them on Http2Session.

Seems like it's worthwhile to ensure unref works on Http2Server as well, so doing both now.

addaleax · 2017-12-12T02:03:39Z

@kjin I think those are different things – ServerHttp2Session corresponds to a single connection, Http2Server to the entire server, right? I’m not sure but they probably both have ref/unref methods?

kjin · 2017-12-12T02:16:09Z

Yep, I understand that ServerHttp2Session objects are created on a per-connection basis by an Http2Server instance, and isn't really an analogue of http.Server. I definitely skipped an explanation step there but what I was intending to say was "no reason why http2 server-side objects shouldn't also have ref and unref".

Currently ServerHttp2Session doesn't have unref, but Http2Server does. ~~I think the latter doesn't do anything, and what I was saying above was that I might as well fix that now (whatever that entails).~~ (edit: it actually does work)

kjin · 2017-12-12T19:30:38Z

@addaleax Thanks for the feedback so far, I've updated the PR to make ref and unref properties of Http2Session instead of ClientHttp2Session (and the docs accordingly). I kept the docs for ClientHttp2Session#request separate for the same reason as in the initial PR description. Can you take another look when you have the chance?

jasnell · 2017-12-12T23:40:50Z

This should wait to land until after #17406, as is may need to be updated based on those changes.

TimothyGu · 2017-12-13T08:19:57Z

@addaleax

Should ClientHttp2Session instances be unrefd by default?

I don’t think so; if you have an active network connection, you usually want that to keep the event loop alive.

Hmm, but if the session isn't transporting any data (just kept open because of HTTP/2 semantics) I don't see a good reason why it should keep the event loop alive.

addaleax · 2017-12-13T08:29:00Z

lib/internal/http2/core.js

+  ref() {
+    if (this[kSocket]) {
+      assert(this[kSocket]._handle);
+      if (typeof this[kSocket]._handle.ref === 'function') {


Why not just this[kSocket].ref? That’s what’s being called anyway, right?

For generic duplex streams this[kSocket] is a JSStreamWrap object, which doesn't have a handle.ref field.

Actually, this makes me think that net.Socket itself should be checking that _handle.ref is a function, rather than here. I'll add a trailing commit that does so, we can drop it if there is already a good rationale for not checking in net.

addaleax · 2017-12-13T08:29:25Z

@TimothyGu This is just calling through to the underlying socket, the HTTP2 session itself won’t keep anything open

kjin · 2017-12-20T22:00:00Z

@jasnell @addaleax I've rebased the PR on master. The only conflict was in the docs, since I moved http2session.request to a new ClientHttp2Session section. The rebased commits should reflect whatever changed in the docs since I first opened this PR.

I can understand if it's not in the scope of this PR to move the request docs, but I still think it makes sense considering that ClientHttp2Session is treated as a separate class in http2/core.js.

jasnell · 2017-12-22T22:46:08Z

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

PR-URL: #17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

jasnell · 2017-12-28T17:56:15Z

landed in 120ea9b

PR-URL: nodejs#17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Backport-PR-URL: #18050 PR-URL: #17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

ofrobots · 2018-04-12T16:48:47Z

@gibfahn This still needs to be released on 8.x. Let myself or @kjin know if there is any work needed from our side to make that happen.

gibfahn · 2018-04-12T18:09:15Z

Thanks for the ping, not sure why this hasn't been backported yet.

kjin · 2018-04-12T18:58:45Z

@gibfahn This PR was originally contingent on #17406 landing, so I assume that the backport will be as well.

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

Backport-PR-URL: #18050 Backport-PR-URL: #20456 PR-URL: #17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Dec 11, 2017

addaleax reviewed Dec 11, 2017

View reviewed changes

kjin force-pushed the http2-client-session-ref branch 2 times, most recently from 826acba to 8714f6f Compare December 12, 2017 19:28

kjin changed the title ~~http2: implement ref() and unref() on client sessions~~ http2: implement ref() and unref() on sessions Dec 12, 2017

addaleax reviewed Dec 13, 2017

View reviewed changes

kjin added 2 commits December 20, 2017 13:47

http2: implement ref() and unref() on client sessions

41b9978

address comments

34f36bb

kjin force-pushed the http2-client-session-ref branch from f601654 to 0346126 Compare December 20, 2017 21:51

kjin added 3 commits December 20, 2017 13:53

make unref a property of http2session

d8c9017

fix lint

55b9fc4

check ref/unref on _handle in net.socket

c64f6b1

kjin force-pushed the http2-client-session-ref branch from 0346126 to c64f6b1 Compare December 20, 2017 21:53

jasnell approved these changes Dec 20, 2017

View reviewed changes

addaleax approved these changes Dec 21, 2017

View reviewed changes

kjin force-pushed the http2-client-session-ref branch from 79739eb to f29e6a7 Compare December 21, 2017 21:08

redo test to use duplex pair instead of PassThrough

b3d2513

kjin force-pushed the http2-client-session-ref branch from f29e6a7 to b3d2513 Compare December 21, 2017 21:19

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2017

jasnell pushed a commit that referenced this pull request Dec 28, 2017

http2: implement ref() and unref() on client sessions

120ea9b

PR-URL: #17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

jasnell closed this Dec 28, 2017

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 29, 2017

MylesBorins pushed a commit to jasnell/node that referenced this pull request Jan 9, 2018

http2: implement ref() and unref() on client sessions

afa4a7c

PR-URL: nodejs#17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins pushed a commit that referenced this pull request Jan 9, 2018

http2: implement ref() and unref() on client sessions

e640e34

Backport-PR-URL: #18050 PR-URL: #17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins pushed a commit that referenced this pull request Jan 10, 2018

http2: implement ref() and unref() on client sessions

1b7ce1e

Backport-PR-URL: #18050 PR-URL: #17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

kjin added a commit to kjin/node that referenced this pull request May 1, 2018

http2: implement ref() and unref() on client sessions

3bf3eb8

Backport-PR-URL: nodejs#18050 PR-URL: nodejs#17620 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins mentioned this pull request May 2, 2018

v8.11.2 proposal #20478

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2: implement ref() and unref() on sessions #17620

http2: implement ref() and unref() on sessions #17620

kjin commented Dec 11, 2017 •

edited

Loading

addaleax left a comment

addaleax Dec 11, 2017

addaleax Dec 11, 2017

kjin commented Dec 12, 2017

addaleax commented Dec 12, 2017

kjin commented Dec 12, 2017 •

edited

Loading

kjin commented Dec 12, 2017 •

edited

Loading

jasnell commented Dec 12, 2017

TimothyGu commented Dec 13, 2017

addaleax Dec 13, 2017

kjin Dec 13, 2017 •

edited

Loading

addaleax commented Dec 13, 2017

kjin commented Dec 20, 2017

jasnell commented Dec 22, 2017

jasnell commented Dec 28, 2017

ofrobots commented Apr 12, 2018

gibfahn commented Apr 12, 2018

kjin commented Apr 12, 2018

http2: implement ref() and unref() on sessions #17620

http2: implement ref() and unref() on sessions #17620

Conversation

kjin commented Dec 11, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

addaleax Dec 11, 2017

Choose a reason for hiding this comment

addaleax Dec 11, 2017

Choose a reason for hiding this comment

kjin commented Dec 12, 2017

addaleax commented Dec 12, 2017

kjin commented Dec 12, 2017 • edited Loading

kjin commented Dec 12, 2017 • edited Loading

jasnell commented Dec 12, 2017

TimothyGu commented Dec 13, 2017

addaleax Dec 13, 2017

Choose a reason for hiding this comment

kjin Dec 13, 2017 • edited Loading

Choose a reason for hiding this comment

addaleax commented Dec 13, 2017

kjin commented Dec 20, 2017

jasnell commented Dec 22, 2017

jasnell commented Dec 28, 2017

ofrobots commented Apr 12, 2018

gibfahn commented Apr 12, 2018

kjin commented Apr 12, 2018

kjin commented Dec 11, 2017 •

edited

Loading

kjin commented Dec 12, 2017 •

edited

Loading

kjin commented Dec 12, 2017 •

edited

Loading

kjin Dec 13, 2017 •

edited

Loading