Skip to content

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

Closed
chrismilleruk wants to merge 1 commit intonodejs:masterfrom
chrismilleruk:issue19711
Closed

doc, http2: add sections for server.close() and note to clarify behavior#19802
chrismilleruk wants to merge 1 commit intonodejs:masterfrom
chrismilleruk:issue19711

Conversation

@chrismilleruk
Copy link
Contributor

@chrismilleruk chrismilleruk commented Apr 4, 2018

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
Copy link
Member

@addaleax addaleax Apr 4, 2018

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

@chrismilleruk chrismilleruk Apr 4, 2018

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
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Apr 4, 2018

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

@chrismilleruk chrismilleruk Apr 4, 2018

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

chrismilleruk commented Apr 4, 2018

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

vsemozhetbyt commented Apr 4, 2018

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 <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>
@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 <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 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 <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 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 <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 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 <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 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 <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

Reviewers

@addaleax addaleax addaleax left review comments

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@trivikr trivikr trivikr approved these changes

+1 more reviewer

@vsemozhetbyt vsemozhetbyt vsemozhetbyt approved these changes

Reviewers whose approvals may not affect merge requirements

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