Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

http2: client should be unref() by default #140

Closed
mcollina opened this issue May 19, 2017 · 6 comments
Closed

http2: client should be unref() by default #140

mcollina opened this issue May 19, 2017 · 6 comments
Assignees

Comments

@mcollina
Copy link
Member

No description provided.

@mcollina mcollina self-assigned this May 19, 2017
@mcollina mcollina changed the title http2: client should be unref() http2: client should be unref() by default May 19, 2017
@c0b
Copy link

c0b commented Oct 24, 2017

agree; i'm writing some code running as http2 client, with current node-v8.7.0 it requires me to call client.destroy() to quit when no other things hold the event loop

vs. http.get( ... ) does not need to worry close the connection.

@apapirovski
Copy link
Member

@c0b I think that's a slightly different issue though because get is one request vs connect which is a whole connection. How does net.connect behave in a similar situation? Also it's better to open issues in the main Node tracker now that http2 is a part of core.

@jasnell
Copy link
Member

jasnell commented Oct 24, 2017

Yep, I should actually close out all of the issues here.

The point still stands tho... we should see if there's some reasonable approach we can take here.

@jasnell jasnell closed this as completed Oct 24, 2017
@apapirovski
Copy link
Member

@jasnell I would say this is a broader issue of the client not matching the http client but in terms of how http2.connect works, this seems right to me since it mimics other places where a persistent connection is established (net.connect or tls.connect).

@addaleax
Copy link
Member

Fun fact, I was actually working on a fix for this this an hour ago or so. I should have something up today or tomorrow :)

@c0b
Copy link

c0b commented Oct 24, 2017

Also it's better to open issues in the main Node tracker now that http2 is a part of core.

or close the whole repo instead? or just close the issues board, leave the remaining at read-only mode for another period before eventually delete

addaleax added a commit to addaleax/node that referenced this issue Oct 30, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Oct 31, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: #16461
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Qard pushed a commit to ayojs/ayo that referenced this issue Nov 2, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this issue Nov 6, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs#16461
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
addaleax added a commit to ayojs/ayo that referenced this issue Dec 7, 2017
Use weak handles to track the lifetime of an `Http2Session` instance.

Refs: nodejs/http2#140
PR-URL: nodejs/node#16461
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants