Http2 timeout documentation by sagitsofan · Pull Request #22798 · nodejs/node

sagitsofan · 2018-09-10T22:19:17Z

Added into documentation new default timeout values of "2 minutes" under 2 classes at "Event: 'timeout'" section.

Class: Http2SecureServer
Class: Http2Server

Documentation:
https://nodejs.org/api/http2.html#http2_event_timeout_2
https://nodejs.org/api/http2.html#http2_event_timeout_3

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Added into documentation new default timeout value of "2 minutes" inside 2 classes under "Event: 'timeout'" 1) Class: Http2SecureServer 2) Class: Http2Server

mscdex · 2018-09-10T23:15:09Z

doc/api/http2.md


 The `'timeout'` event is emitted when there is no activity on the Server for
 a given number of milliseconds set using `http2server.setTimeout()`.
+**Default:** `2 Minutes`.


I would remove the backticks here, those are typically used to enclose a class name or a literal (JS) value. Also, 'Minutes' should probably be lowercased.

removed backticks from default value

lower casing

sagitsofan · 2018-09-11T05:14:49Z

fixed

apapirovski

Thanks for working on this. It would probably be better if the location of these defaults were more similar to the http docs: https://nodejs.org/api/http.html#http_server_timeout

Of course, these sections currently don't exist and they should also probably be created.

sagitsofan · 2018-09-11T16:08:52Z

Sure. working on it.

added new section for setTimeout method inside Http2SecureServer & Http2Server docs

sagitsofan · 2018-09-11T18:57:24Z

Please review

vsemozhetbyt

Thank you for improving docs!

Just some nits)

vsemozhetbyt · 2018-09-11T19:17:56Z

doc/api/http2.md

 a given number of milliseconds set using `http2server.setTimeout()`.
+**Default:** 2 minutes.
+
+#### server.setTimeout([msecs][, callback])


Section headings are ABC-sorted, so this and the next added headings need to be placed after the #### server.close([callback]).

If an error is thrown if no callback is assigned, should we make the callback parameter in both signatures mandatory?

vsemozhetbyt · 2018-09-11T19:32:39Z

doc/api/http2.md

+* `callback` {Function}
+* Returns: {Http2Server}
+
+Used to set the timeout value for http2 secure server requests, 


http2 secure server -> http2 server

vsemozhetbyt · 2018-09-11T19:32:56Z

doc/api/http2.md

+
+Used to set the timeout value for http2 secure server requests, 
+and sets a callback function that is called when there is no activity 
+on the Http2Server after `msecs` milliseconds. 


Http2Server -> `Http2Server`.

vsemozhetbyt · 2018-09-11T19:33:34Z

doc/api/http2.md

+and sets a callback function that is called when there is no activity 
+on the Http2Server after `msecs` milliseconds. 
+
+The given callback is registered as a listener on the 'timeout' event.


'timeout' -> `'timeout'`

vsemozhetbyt · 2018-09-11T19:34:27Z

doc/api/http2.md

+
+Used to set the timeout value for http2 secure server requests, 
+and sets a callback function that is called when there is no activity 
+on the Http2Server after `msecs` milliseconds. 


Http2SecureServer -> `Http2SecureServer`

vsemozhetbyt · 2018-09-11T19:36:46Z

doc/api/http2.md

+The given callback is registered as a listener on the 'timeout' event.
+
+In case of no callback were assigned, a new `ERR_INVALID_CALLBACK` 
+error will be throw.


throw -> thrown

vsemozhetbyt · 2018-09-11T19:37:16Z

doc/api/http2.md

+and sets a callback function that is called when there is no activity 
+on the Http2Server after `msecs` milliseconds. 
+
+The given callback is registered as a listener on the 'timeout' event.


'timeout' -> `'timeout'`

vsemozhetbyt · 2018-09-11T19:38:23Z

doc/api/http2.md

+The given callback is registered as a listener on the 'timeout' event.
+
+In case of no callback were assigned, a new `ERR_INVALID_CALLBACK` 
+error will be throw.


throw -> thrown

vsemozhetbyt · 2018-09-11T19:59:56Z

doc/api/http2.md

+* `callback` {Function}
+* Returns: {Http2SecureServer}
+
+Used to set the timeout value for http2 server requests, 


Sorry, in this section, http2 secure server seemed appropriate)

vsemozhetbyt · 2018-09-11T20:00:31Z

doc/api/http2.md

+
+Used to set the timeout value for http2 server requests, 
+and sets a callback function that is called when there is no activity 
+on the `Http2Server` after `msecs` milliseconds. 


`Http2Server` -> `Http2SecureServer` :)

vsemozhetbyt · 2018-09-11T20:01:42Z

сс @nodejs/http2

refack · 2018-09-11T20:01:59Z

Hello @sagitsofan and thank you for the contribution.
If you haven't already, it's recommended you take a look at CONTRIBUTING.md guide (especially the part about "discuss and update"). Customarily PRs are kept open for at least 48 hour so that anyone interested gets a chance to comment or review.
Also documentation changes, while easy to do are more difficult to validate. With code we can write tests and the CI can validate the change. With documentation the only way to validate is with human eyes, so don't be dismayed if you get a lot of feedback.
Anyway thank you for the contribution, and good luck 🥇

P.S. If you have any question you can also feel free to contact me directly & בהצלחה

sagitsofan · 2018-09-11T20:14:32Z

Thank you @refack appreciate this

mcollina · 2018-09-12T07:01:01Z

For posterity, here is the line defining the 2 minutes timeout:

node/lib/internal/http2/core.js

Line 163 in b36c581

const kDefaultSocketTimeout = 2 * 60 * 1000;

.

mcollina

LGTM

vsemozhetbyt · 2018-09-13T15:24:38Z

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

New default timeout values of "2 minutes" were added into documentation inside 2 classes under "Event: 'timeout'": 1) Class: Http2SecureServer 2) Class: Http2Server New sections for `.setTimeout()` method were added inside `Http2SecureServer` & `Http2Server` docs. PR-URL: #22798 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

vsemozhetbyt · 2018-09-13T16:11:32Z

Landed in 19c0620
Thank you!

sagitsofan · 2018-09-13T16:21:37Z

Closed but without merging, why is that?

vsemozhetbyt · 2018-09-13T16:35:14Z

It is merged, but in some other way, not with GitHub interface. See more about this in https://github.com/nodejs/node/blob/master/COLLABORATOR_GUIDE.md#landing-pull-requests

vsemozhetbyt · 2018-09-13T16:39:43Z

See also a note in https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#step-10-landing

New default timeout values of "2 minutes" were added into documentation inside 2 classes under "Event: 'timeout'": 1) Class: Http2SecureServer 2) Class: Http2Server New sections for `.setTimeout()` method were added inside `Http2SecureServer` & `Http2Server` docs. PR-URL: #22798 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

New default timeout values of "2 minutes" were added into documentation inside 2 classes under "Event: 'timeout'": 1) Class: Http2SecureServer 2) Class: Http2Server New sections for `.setTimeout()` method were added inside `Http2SecureServer` & `Http2Server` docs. PR-URL: nodejs#22798 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

New default timeout values of "2 minutes" were added into documentation inside 2 classes under "Event: 'timeout'": 1) Class: Http2SecureServer 2) Class: Http2Server New sections for `.setTimeout()` method were added inside `Http2SecureServer` & `Http2Server` docs. Backport-PR-URL: #22850 PR-URL: #22798 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Http2 timeout documentation

93a39e7

Added into documentation new default timeout value of "2 minutes" inside 2 classes under "Event: 'timeout'" 1) Class: Http2SecureServer 2) Class: Http2Server

nodejs-github-bot added doc Issues and PRs related to the documentations. http2 Issues or PRs related to the http2 subsystem. labels Sep 10, 2018

sagitsofan mentioned this pull request Sep 10, 2018

http2 server default timeout? #22789

Closed

mscdex reviewed Sep 10, 2018

View reviewed changes

sagitsofan added 2 commits September 11, 2018 07:46

backticks

a47d0ab

removed backticks from default value

lower casing

8d409ce

lower casing

apapirovski reviewed Sep 11, 2018

View reviewed changes

sagitsofan added 2 commits September 11, 2018 21:41

added new section

f703eb0

added new section for setTimeout method inside Http2SecureServer & Http2Server docs

fix typo

30678e9

row max length fix

885ecf9

vsemozhetbyt reviewed Sep 11, 2018

View reviewed changes

sagitsofan added 2 commits September 11, 2018 22:50

typos & order

acda854

added function word

5ac898c

vsemozhetbyt reviewed Sep 11, 2018

View reviewed changes

words changing 👍

c749d6e

mcollina approved these changes Sep 12, 2018

View reviewed changes

cjihrig approved these changes Sep 12, 2018

View reviewed changes

jasnell approved these changes Sep 12, 2018

View reviewed changes

vsemozhetbyt closed this Sep 13, 2018

sagitsofan deleted the my-branch branch September 14, 2018 11:23

sagitsofan restored the my-branch branch September 15, 2018 08:05

sagitsofan deleted the my-branch branch September 15, 2018 17:16

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

BethGriggs mentioned this pull request Oct 30, 2018

v8.13.0 proposal #23974

Merged

beevelop mentioned this pull request Nov 20, 2018

2018-11-20 Version 8.13.0 'Carbon' (LTS) @MylesBorins prepared by @BethGriggs beevelop/docker-android-nodejs#250

Closed

Uh oh!

Conversation

sagitsofan commented Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

mscdex Sep 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sagitsofan commented Sep 11, 2018

Uh oh!

apapirovski left a comment

Choose a reason for hiding this comment

Uh oh!

sagitsofan commented Sep 11, 2018

Uh oh!

sagitsofan commented Sep 11, 2018

Uh oh!

vsemozhetbyt left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Sep 11, 2018

Uh oh!

refack commented Sep 11, 2018

Uh oh!

sagitsofan commented Sep 11, 2018

Uh oh!

mcollina commented Sep 12, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

vsemozhetbyt commented Sep 13, 2018

Uh oh!

vsemozhetbyt commented Sep 13, 2018

Uh oh!

sagitsofan commented Sep 13, 2018

Uh oh!

vsemozhetbyt commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsemozhetbyt commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

sagitsofan commented Sep 10, 2018 •

edited

Loading

mscdex Sep 10, 2018 •

edited

Loading

vsemozhetbyt Sep 11, 2018 •

edited

Loading

vsemozhetbyt Sep 11, 2018 •

edited

Loading

vsemozhetbyt commented Sep 13, 2018 •

edited

Loading