-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
configurable global socket timeouts #31603
Conversation
Pinging @elastic/kibana-operations |
💔 Build Failed |
💚 Build Succeeded |
💚 Build Succeeded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I've only looked at the code. Will take a stab at testing this tomorrow, and I think we should add some docs for this config values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I have no idea how to test this. But the usage lines up with both the hapi and node http server docs, so I'm good with it.
server.listener.setTimeout(listenerOptions.socketTimeout); | ||
server.listener.on('timeout', socket => { | ||
if (socket.writable) { | ||
socket.end(Buffer.from('HTTP/1.1 408 Request Timeout\r\n\r\n', 'ascii')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
adding a test for timeout shouldn't be a problem. in http_server.test.ts:
const delay = (ms: number) => new Promise(res => setTimeout(res, ms));
test('returns 408 on timeout error', async () => {
const router = new Router('');
router.get({ path: '/', validate: false }, async (req, res) => {
await delay(1000);
return res.ok({});
});
const { registerRouter, server: innerServer } = await server.setup({
...config,
socketTimeout: 1,
});
registerRouter(router);
await server.start();
await supertest(innerServer.listener).get('/').expect(408)
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added!
💚 Build Succeeded |
💔 Build Failed |
retest |
💚 Build Succeeded |
* configurable global socket timeouts * update snapshots * update tests * add test * add test * add test * happy path * test happy path * docs * stop server after
* configurable global socket timeouts * update snapshots * update tests * add test * add test * add test * happy path * test happy path * docs * stop server after
* configurable global socket timeouts * update snapshots * update tests * add test * add test * add test * happy path * test happy path * docs * stop server after
* configurable global socket timeouts * update snapshots * update tests * add test * add test * add test * happy path * test happy path * docs * stop server after
* configurable global socket timeouts (#31603) * configurable global socket timeouts * update snapshots * update tests * add test * add test * add test * happy path * test happy path * docs * stop server after * fix/lint import ordering * use older new platform apis * prettier * config => HttpConfig
@jbudz @peterschretlen it seems this has been reverted on 6.8 branch again. Is there plans to reintroduce this, otherwise I think we should remove the 6.8.2 label from this PR. |
This introduces two new kibana.yml configurations:
server.socketTimeout
: default 2 minutes, close sockets after this amount of inactivityserver.keepAliveTimeout
: default 2 minutes, close keepalive sockets after this amount of inactivityCloses #31549