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

doc,test: add server.timeout property to http2 public API #31693

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
doc,test: add server.timeout property to http2 public API
Both http and https modules have server.timeout property
in public API. This commit adds documentation section and test
for server.timeout in http2 module, so it becomes consistent
with http and https.

Also improves description of callback argument in documentation
for server.setTimeout().
  • Loading branch information
puzpuzpuz committed Mar 6, 2020
commit 563c0ab968fac9657a9e9e57ad035f8a1d1621e9
42 changes: 40 additions & 2 deletions doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -1797,9 +1797,28 @@ on the `Http2Server` after `msecs` milliseconds.

The given callback is registered as a listener on the `'timeout'` event.

In case of no callback function were assigned, a new `ERR_INVALID_CALLBACK`
In case if `callback` is not a function, a new `ERR_INVALID_CALLBACK`
error will be thrown.

#### `server.timeout`
<!-- YAML
added: v8.4.0
changes:
- version: v13.0.0
pr-url: https://github.com/nodejs/node/pull/27558
description: The default timeout changed from 120s to 0 (no timeout).
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
description: The default timeout changed from 120s to 0 (no timeout).
description: The default timeout changed from 120 seconds to 0 (no timeout).

here and below

Copy link
Member Author

Choose a reason for hiding this comment

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

The same text (120s) is also used in http.md and will be used in https.md once #31692 lands. So, it's better to wait until that PR lands and change text in all places simultaneously.

-->

* {number} Timeout in milliseconds. **Default:** 0 (no timeout)

The number of milliseconds of inactivity before a socket is presumed
to have timed out.

A value of `0` will disable the timeout behavior on incoming connections.

The socket timeout logic is set up on connection, so changing this
value only affects new connections to the server, not any existing connections.

### Class: `Http2SecureServer`
<!-- YAML
added: v8.4.0
Expand Down Expand Up @@ -1943,9 +1962,28 @@ on the `Http2SecureServer` after `msecs` milliseconds.

The given callback is registered as a listener on the `'timeout'` event.

In case of no callback function were assigned, a new `ERR_INVALID_CALLBACK`
In case if `callback` is not a function, a new `ERR_INVALID_CALLBACK`
error will be thrown.

#### `server.timeout`
<!-- YAML
added: v8.4.0
changes:
- version: v13.0.0
pr-url: https://github.com/nodejs/node/pull/27558
description: The default timeout changed from 120s to 0 (no timeout).
-->

* {number} Timeout in milliseconds. **Default:** 0 (no timeout)

The number of milliseconds of inactivity before a socket is presumed
to have timed out.

A value of `0` will disable the timeout behavior on incoming connections.

The socket timeout logic is set up on connection, so changing this
value only affects new connections to the server, not any existing connections.

### `http2.createServer(options[, onRequestHandler])`
<!-- YAML
added: v8.4.0
Expand Down
34 changes: 20 additions & 14 deletions test/parallel/test-http2-server-timeout.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,27 @@ if (!common.hasCrypto)
common.skip('missing crypto');
const http2 = require('http2');

const server = http2.createServer();
server.setTimeout(common.platformTimeout(50));
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we keep this as a separate test in this file since server.setTimeout() should still work?

Copy link
Member Author

Choose a reason for hiding this comment

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

At first I've moved it into a separate test, but I agree that it doesn't make a lot of sense in this case. Introduced additional assert in this very test.

Copy link
Member

Choose a reason for hiding this comment

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

I think what was meant was a separate test file. Calling server.setTimeout() then immediately resetting it with server.timeout does not really ensure that server.setTimeout() does the right thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Original version of this PR included a separate test, but I have removed it and merged everything into a this test after receiving this comment.

@mscdex could you check current code and tell me if that's what you wanted?

Copy link
Member Author

Choose a reason for hiding this comment

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

@mscdex could you check current code and tell me if that's what you wanted? I'm thinking of reverting this change, as it seems more logical to me to keep tests isolated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the test, so now it verifies both timeout property and setTimeout method in isolation from each other. Hopefully, it resolves this comment.

function testServerTimeout(setTimeoutFn) {
const server = http2.createServer();
setTimeoutFn(server);

const onServerTimeout = common.mustCall((session) => {
session.close();
});
const onServerTimeout = common.mustCall((session) => {
session.close();
});

server.on('stream', common.mustNotCall());
server.once('timeout', onServerTimeout);
server.on('stream', common.mustNotCall());
server.once('timeout', onServerTimeout);

server.listen(0, common.mustCall(() => {
const url = `http://localhost:${server.address().port}`;
const client = http2.connect(url);
client.on('close', common.mustCall(() => {
const client2 = http2.connect(url);
client2.on('close', common.mustCall(() => server.close()));
server.listen(0, common.mustCall(() => {
const url = `http://localhost:${server.address().port}`;
const client = http2.connect(url);
client.on('close', common.mustCall(() => {
const client2 = http2.connect(url);
client2.on('close', common.mustCall(() => server.close()));
}));
}));
}));
}

const timeout = common.platformTimeout(50);
testServerTimeout((server) => server.setTimeout(timeout));
testServerTimeout((server) => server.timeout = timeout);