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

doc, http2: add sections for server.close() and note to clarify behavior #19802

Closed
wants to merge 1 commit into from
Closed

doc, http2: add sections for server.close() and note to clarify behavior #19802

wants to merge 1 commit into from

Conversation

chrismilleruk
Copy link
Contributor

Clarify current behavior of https2server.close() and http2secureServer.close()
w.r.t. perceived differences when compared with httpServer.close().

Fixes: #19711

I'm open to discussion on whether this is the right approach / wording. Thanks in advance for any help :-)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Apr 4, 2018
addaleax
addaleax reviewed Apr 4, 2018
View reviewed changes
doc/api/http2.md Outdated
@@ -1645,6 +1657,18 @@ negotiate an allowed protocol (i.e. HTTP/2 or HTTP/1.1). The event handler
receives the socket for handling. If no listener is registered for this event,
the connection is terminated. See the [Compatibility API][].

#### server.close([callback])
<!-- YAML
added: v0.3.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make sure that these versions are right? I think it’s 8.4.0, the version that the whole http2 module was originally introduced in

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that makes sense. Will fix.

vsemozhetbyt
vsemozhetbyt reviewed Apr 4, 2018
View reviewed changes
doc/api/http2.md Outdated
@@ -3137,4 +3163,5 @@ following additional properties:
[`tls.TLSSocket`]: tls.html#tls_class_tls_tlssocket
[`tls.connect()`]: tls.html#tls_tls_connect_options_callback
[`tls.createServer()`]: tls.html#tls_tls_createserver_options_secureconnectionlistener
[`tls.Server.close()`]: tls.html#tls_server_close_callback
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A nit: it seems this need to go before [`tls.TLSSocket`]:, these references are sorted in ASCII order.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, yep I can sort.

vsemozhetbyt
vsemozhetbyt approved these changes Apr 4, 2018
View reviewed changes
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc format LGTM, with @addaleax's comment addressed)

@chrismilleruk
doc, http2: add sections for server.close() and note to clarify behavior
e540786 
Clarify current behavior of http2server.close() and http2secureServer.close()
w.r.t. perceived differences when compared with httpServer.close().

Fixes: nodejs#19711
@chrismilleruk
Copy link
Contributor Author

Review comments addressed and PR updated.

@vsemozhetbyt vsemozhetbyt added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 4, 2018
@vsemozhetbyt
Copy link
Contributor

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

@vsemozhetbyt vsemozhetbyt added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 4, 2018
trivikr
trivikr approved these changes Apr 4, 2018
View reviewed changes
Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, verified at private branch

cc @nodejs/http2

@trivikr trivikr removed the fast-track PRs that do not need to wait for 48 hours to land. label Apr 4, 2018
jasnell
jasnell approved these changes Apr 4, 2018
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

mcollina
mcollina approved these changes Apr 5, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@trivikr trivikr added the fast-track PRs that do not need to wait for 48 hours to land. label Apr 5, 2018
trivikr pushed a commit that referenced this pull request Apr 7, 2018
@chrismilleruk @trivikr
doc, http2: add sections for server.close()
c60c93c 
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@trivikr
Copy link
Member

trivikr commented Apr 7, 2018

Landed in c60c93c

Congrats @chrismilleruk for your first commit in Node.js core! 🎉🎉🎉

@trivikr trivikr closed this Apr 7, 2018
targos pushed a commit that referenced this pull request Apr 12, 2018
@chrismilleruk @targos
doc, http2: add sections for server.close()
a466021 
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request May 1, 2018
@chrismilleruk @kjin
doc, http2: add sections for server.close()
7d3ff81 
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 2, 2018
@chrismilleruk @MylesBorins
doc, http2: add sections for server.close()
3a890a8 
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 2, 2018
Merged
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@chrismilleruk @MylesBorins
doc, http2: add sections for server.close()
88d4794 
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2018
@chrismilleruk @MylesBorins
doc, http2: add sections for server.close()
84fa6eb 
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 <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@trivikr trivikr trivikr approved these changes

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. doc Issues and PRs related to the documentations. fast-track PRs that do not need to wait for 48 hours to land. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Http2SecureServer.close fails
7 participants
@chrismilleruk @vsemozhetbyt @trivikr @mcollina @jasnell @addaleax @nodejs-github-bot