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.

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

Http2SecureServer.close fails #19711

Closed
aabfred opened this issue Mar 31, 2018 · 7 comments
Closed

Http2SecureServer.close fails #19711

aabfred opened this issue Mar 31, 2018 · 7 comments
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@aabfred
Copy link

aabfred commented Mar 31, 2018

  • Version: v9.10.1
  • Platform: Linux Host-001 4.14.6-300.fc27.x86_64 deps: update openssl to 1.0.1j #1 SMP Thu Dec 14 15:31:24 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: http2

Problem:
Closing an Http2Server runs only when it has no connection;
Otherwise, no "close" or "error" event is emitted after close(), and script doesn't exit (server still listening)

bug.js:
const
http2 = require( "http2" ),
srv = http2.createServer();
srv.listen(9000);
const client = http2.connect("http://localhost:9000");
setTimeout(() => { srv.close() }, 500);

$ export NODE_DEBUG=http2; node ./bug.js
HTTP2 30406: Http2Session client: created
(node:30406) ExperimentalWarning: The http2 module is an experimental API.
HTTP2 30406: Http2Session server: received a connection
HTTP2 30406: Http2Session server: setting up session handle
HTTP2 30406: Http2Session server: sending settings
HTTP2 30406: Http2Session server: submitting settings
HTTP2 30406: Http2Session server: created
HTTP2 30406: Http2Session client: setting up session handle
HTTP2 30406: Http2Session client: sending settings
HTTP2 30406: Http2Session client: submitting settings
HTTP2 30406: Http2Session client: new settings received
HTTP2 30406: Http2Session server: new settings received
HTTP2 30406: Http2Session server: settings received
HTTP2 30406: Http2Session client: settings received

@anliting
Copy link

anliting commented Apr 1, 2018

Maybe it is not a bug: "Stops the server from accepting new connections and keeps existing connections." (from https://nodejs.org/api/net.html#net_server_close_callback)

@aabfred
Copy link
Author

aabfred commented Apr 2, 2018

Http2Server.close() has not the same behaviour as HttpServer.close() and HttpsServer.close().
I'm not opposed to this idea, it sould then be explicit in http2 docs.

@anliting
Copy link

anliting commented Apr 2, 2018

I think they have the same behavior, consider:

import http from 'http'
let sv=http.createServer((rq,rs)=>{})
sv.listen(8000)
let rq=http.get('http://localhost:8000/',rs=>{
    sv.close()
})

@lpinca
Copy link
Member

lpinca commented Apr 2, 2018

http2.createServer() and http2.createSecureServer() return respectively an instance of Http2Server and Http2SecureServer which inherit from net.Server and tls.Server so it is somehow implied that the close method work in the same way and has the same behavior.

That said, it doesn't harm to clarify that in the docs.

@lpinca lpinca added doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors. labels Apr 2, 2018
@anliting
Copy link

anliting commented Apr 2, 2018

I would guess @aabfred is new to HTTP/2. Would you like to figure the differences out, and describe the illusion that they don't have the same behavior? I think it might help making a good documentation.

@chrismilleruk
Copy link
Contributor

Hi, I'm interested in making some improvements here if this is available?

I noticed that inherited methods such as close don't appear in the HTTP2 docs, is that a deliberate style-guide convention or is it OK to add these in with a suitable footnote?

I also thought it might be worth introducing a brief explainer near the top to cover the most obvious differences (i.e. introduction of sessions and streams). Would be keen to hear thoughts on this.

@chrismilleruk
Copy link
Contributor

OK, PR sent. I've kept it simple for the moment but I have enough context now that I can add more details if needed.

The perceived behaviour seems to be that server.close() would result in process termination within a reasonable timescale. This is true for HTTP/1 where connections and requests have a reasonably close relationship but less true for HTTP/2 where connections are persistent and clients are encouraged to keep them open for long periods.

I think that calling http2Session.close() on all active sessions will help to gracefully close open sockets so I made a suggestion in the footnotes but I may be wrong (looking to validate this).

💭 if calling session.close() on all children is a common use case, perhaps this should be default behaviour? I saw a mention of an http2.Pool object in #17746 that might make this easier to implement.

chrismilleruk added a commit to chrismilleruk/node that referenced this issue Apr 4, 2018
Clarify current behavior of http2server.close() and http2secureServer.close()
w.r.t. perceived differences when compared with httpServer.close().

Fixes: nodejs#19711
@trivikr trivikr closed this as completed in c60c93c Apr 7, 2018
targos pushed a commit that referenced this issue Apr 12, 2018
Clarify current behavior of http2server.close() and
http2secureServer.close() w.r.t. perceived differences
when compared with httpServer.close().

Fixes: #19711

PR-URL: #19802
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
kjin pushed a commit to kjin/node that referenced this issue May 1, 2018
Clarify current behavior of http2server.close() and
http2secureServer.close() w.r.t. perceived differences
when compared with httpServer.close().

Fixes: nodejs#19711

PR-URL: nodejs#19802
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue May 2, 2018
Clarify current behavior of http2server.close() and
http2secureServer.close() w.r.t. perceived differences
when compared with httpServer.close().

Fixes: #19711

Backport-PR-URL: #20456
PR-URL: #19802
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue May 15, 2018
Clarify current behavior of http2server.close() and
http2secureServer.close() w.r.t. perceived differences
when compared with httpServer.close().

Fixes: #19711

Backport-PR-URL: #20456
PR-URL: #19802
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this issue May 15, 2018
Clarify current behavior of http2server.close() and
http2secureServer.close() w.r.t. perceived differences
when compared with httpServer.close().

Fixes: #19711

Backport-PR-URL: #20456
PR-URL: #19802
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants