Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

http2 - clarify difference between 'streamClosed' and 'close' #15303

Closed
ronag opened this issue Sep 10, 2017 · 4 comments
Closed

http2 - clarify difference between 'streamClosed' and 'close' #15303

ronag opened this issue Sep 10, 2017 · 4 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@ronag
Copy link
Member

ronag commented Sep 10, 2017

This is not really well documented. Even comments in the code just say "distinctly different". I'm mostly interested in how it affects pump which seems to be using close instead of streamClosed for http2 requests.
@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 10, 2017
@ronag
Copy link
Member Author

ronag commented Sep 14, 2017

ping?

@gibfahn
Copy link
Member

gibfahn commented Sep 17, 2017

cc/ @nodejs/http2

@mcollina
Copy link
Member

I think this is a bug.

@jasnell why are we using 'streamClosed' instead of 'close' in https://github.com/nodejs/node/blob/master/lib/internal/http2/core.js#L1527?

@jasnell
Copy link
Member

jasnell commented Sep 18, 2017

I added 'streamClosed' because at the time it was not clear if we should tie the protocol idea of a closed stream to the existing 'close' event used for the Streams API. One difference in particular is that 'streamClosed' will provide the protocol error number as reported by the RST_STREAM frame. These likely could be unified in some way but I have not taken the time to look into it further.

@ronag ronag mentioned this issue Sep 19, 2017
Closed
@jasnell jasnell mentioned this issue Nov 26, 2017
Closed
4 tasks
jasnell added a commit that referenced this issue Nov 28, 2017
@jasnell
http2: general cleanups
d4111d0 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
jasnell added a commit that referenced this issue Nov 28, 2017
@jasnell
http2: reduce code duplication in settings
df33d8d 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
jasnell added a commit that referenced this issue Nov 28, 2017
@jasnell
http2: use 'close' event instead of 'streamClosed'
1b99542 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
jasnell added a commit that referenced this issue Nov 28, 2017
@jasnell
test: fixup test-http2-create-client-secure-session
5fe111a 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
jasnell added a commit that referenced this issue Nov 28, 2017
@jasnell
http2: remove unnecessary event handlers
973b8e0 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
jasnell added a commit that referenced this issue Nov 28, 2017
@jasnell
http2: use more descriptive names
093a870 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 7, 2017
@jasnell @MylesBorins
http2: use correct connect event for TLS Socket
d607718 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 7, 2017
@jasnell @MylesBorins
http2: use correct connect event for TLS Socket
c81e968 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
evanlucas pushed a commit that referenced this issue Dec 7, 2017
@jasnell @evanlucas
http2: use correct connect event for TLS Socket
71cf21a 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 7, 2017
@jasnell @MylesBorins
http2: use 'close' event instead of 'streamClosed'
8979e0d 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 7, 2017
@jasnell @MylesBorins
http2: use 'close' event instead of 'streamClosed'
84620d8 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 8, 2017
@jasnell @MylesBorins
http2: use 'close' event instead of 'streamClosed'
aba3544 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 8, 2017
@jasnell @MylesBorins
http2: use correct connect event for TLS Socket
42f09ed 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 8, 2017
@jasnell @MylesBorins
http2: use 'close' event instead of 'streamClosed'
65f209c 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 8, 2017
@jasnell @MylesBorins
http2: use correct connect event for TLS Socket
f0f9e1a 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: general cleanups
fa17337 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: reduce code duplication in settings
f7b1454 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
test: fixup test-http2-create-client-secure-session
7598f8a 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: use more descriptive names
4bc22c0 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: general cleanups
2b06f74 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: reduce code duplication in settings
4ab425b 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
test: fixup test-http2-create-client-secure-session
9b3261c 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: remove unnecessary event handlers
fbf94d4 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: use more descriptive names
0e32fe7 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: general cleanups
e5f92cd 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: reduce code duplication in settings
67abc1e 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
test: fixup test-http2-create-client-secure-session
047bac2 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: remove unnecessary event handlers
4994d57 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 12, 2017
@jasnell @MylesBorins
http2: use more descriptive names
658338e 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 19, 2017
@jasnell @MylesBorins
http2: general cleanups
bdcbe81 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 19, 2017
@jasnell @MylesBorins
http2: reduce code duplication in settings
31cdbd6 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 19, 2017
@jasnell @MylesBorins
test: fixup test-http2-create-client-secure-session
927a281 
General improvements to test and verify that a secureConnect
handler is present

PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 19, 2017
@jasnell @MylesBorins
http2: remove unnecessary event handlers
9fd0b32 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
MylesBorins pushed a commit that referenced this issue Dec 19, 2017
@jasnell @MylesBorins
http2: use more descriptive names
673e263 
PR-URL: #17328
Fixes: #15303
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Sebastiaan Deckers <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

http2: multiple cleanups and s/streamClosed/close jasnell/node
5 participants
@mcollina @mscdex @jasnell @ronag @gibfahn