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

Http2 timeout documentation #22798

Closed
wants to merge 9 commits into from
Closed

Conversation

sagitsofan
Copy link
Contributor

@sagitsofan sagitsofan commented Sep 10, 2018

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

  1. Class: Http2SecureServer
  2. 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
@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 Sep 10, 2018
doc/api/http2.md Outdated
@@ -1615,6 +1615,7 @@ added: v8.4.0

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

@mscdex mscdex Sep 10, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

removed backticks from default value
lower casing
@sagitsofan
Copy link
Contributor Author

fixed

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

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

Sure. working on it.

added new section for setTimeout method inside Http2SecureServer & Http2Server docs
@sagitsofan
Copy link
Contributor Author

Please review

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.

Thank you for improving docs!

Just some nits)

doc/api/http2.md Outdated
@@ -1615,6 +1615,22 @@ added: v8.4.0

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.

#### server.setTimeout([msecs][, callback])
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Sep 11, 2018

Choose a reason for hiding this comment

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

Section headings are ABC-sorted, so this and the next added headings need to be placed after the #### 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.

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

doc/api/http2.md Outdated
* `callback` {Function}
* Returns: {Http2Server}

Used to set the timeout value for http2 secure server requests,
Copy link
Contributor

Choose a reason for hiding this comment

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

http2 secure server -> http2 server

doc/api/http2.md Outdated

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

Choose a reason for hiding this comment

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

Http2Server -> `Http2Server`.

doc/api/http2.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

'timeout' -> `'timeout'`

doc/api/http2.md Outdated

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

Choose a reason for hiding this comment

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

Http2SecureServer -> `Http2SecureServer`

doc/api/http2.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

throw -> thrown

doc/api/http2.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

'timeout' -> `'timeout'`

doc/api/http2.md Outdated
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.
Copy link
Contributor

Choose a reason for hiding this comment

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

throw -> thrown

doc/api/http2.md Outdated
* `callback` {Function}
* Returns: {Http2SecureServer}

Used to set the timeout value for http2 server requests,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

doc/api/http2.md Outdated

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

@vsemozhetbyt vsemozhetbyt Sep 11, 2018

Choose a reason for hiding this comment

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

`Http2Server` -> `Http2SecureServer` :)

@vsemozhetbyt
Copy link
Contributor

сс @nodejs/http2

@refack
Copy link
Contributor

refack commented Sep 11, 2018

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

Thank you @refack appreciate this

@mcollina
Copy link
Member

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

const kDefaultSocketTimeout = 2 * 60 * 1000;
.

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

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt pushed a commit that referenced this pull request Sep 13, 2018
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
Copy link
Contributor

Landed in 19c0620
Thank you!

@sagitsofan
Copy link
Contributor Author

Closed but without merging, why is that?

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 13, 2018

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

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

@sagitsofan sagitsofan deleted the my-branch branch September 14, 2018 11:23
targos pushed a commit that referenced this pull request Sep 14, 2018
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>
@sagitsofan sagitsofan restored the my-branch branch September 15, 2018 08:05
@sagitsofan sagitsofan deleted the my-branch branch September 15, 2018 17:16
targos pushed a commit that referenced this pull request Sep 19, 2018
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>
targos pushed a commit that referenced this pull request Sep 20, 2018
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>
kjin pushed a commit to kjin/node that referenced this pull request Oct 3, 2018
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>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
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>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
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>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
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. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants