From 6ce7f1ebae4220ce78cceebe47738791a5a48e77 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 29 Jul 2020 10:12:21 -0700 Subject: [PATCH 01/26] Route options timeout -> timeout.payload --- src/core/server/http/http_server.test.ts | 124 ++++++++++++++---- src/core/server/http/http_server.ts | 14 +- src/core/server/http/router/request.ts | 9 +- src/core/server/http/router/route.ts | 10 +- .../server/routes/import_list_item_route.ts | 4 +- 5 files changed, 119 insertions(+), 42 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 007d75a69b955..2348dd789482d 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -799,6 +799,7 @@ test('exposes route details of incoming request to a route handler', async () => authRequired: true, xsrfRequired: false, tags: [], + timeout: {}, }, }); }); @@ -906,6 +907,9 @@ test('exposes route details of incoming request to a route handler (POST + paylo authRequired: true, xsrfRequired: true, tags: [], + timeout: { + payload: 10000, + }, body: { parse: true, // hapi populates the default maxBytes: 1024, // hapi populates the default @@ -993,7 +997,7 @@ describe('body options', () => { }); describe('timeout options', () => { - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a POST', async () => { + test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a POST', async () => { const { registerRouter, server: innerServer } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); @@ -1001,11 +1005,19 @@ describe('timeout options', () => { { path: '/', validate: false, - options: { timeout: 300000 }, + options: { + timeout: { + payload: 300000, + }, + }, }, (context, req, res) => { try { - return res.ok({ body: { timeout: req.route.options.timeout } }); + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); } catch (err) { return res.internalError({ body: err.message }); } @@ -1013,12 +1025,18 @@ describe('timeout options', () => { ); registerRouter(router); await server.start(); - await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, { - timeout: 300000, - }); + await supertest(innerServer.listener) + .post('/') + .send({ test: 1 }) + .expect(200, { + timeout: { + payload: 300000, + idleSocket: 300001, + }, + }); }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a GET', async () => { + test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a GET', async () => { const { registerRouter, server: innerServer } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); @@ -1026,11 +1044,19 @@ describe('timeout options', () => { { path: '/', validate: false, - options: { timeout: 300000 }, + options: { + timeout: { + payload: 300000, + }, + }, }, (context, req, res) => { try { - return res.ok({ body: { timeout: req.route.options.timeout } }); + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); } catch (err) { return res.internalError({ body: err.message }); } @@ -1039,11 +1065,11 @@ describe('timeout options', () => { registerRouter(router); await server.start(); await supertest(innerServer.listener).get('/').expect(200, { - timeout: 300000, + timeout: {}, }); }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a DELETE', async () => { + test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a DELETE', async () => { const { registerRouter, server: innerServer } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); @@ -1051,11 +1077,22 @@ describe('timeout options', () => { { path: '/', validate: false, - options: { timeout: 300000 }, + options: { + timeout: { + payload: 300000, + }, + }, }, (context, req, res) => { try { - return res.ok({ body: { timeout: req.route.options.timeout } }); + return res.ok({ + body: { + timeout: { + payload: 300000, + idleSocket: 300001, + }, + }, + }); } catch (err) { return res.internalError({ body: err.message }); } @@ -1063,12 +1100,17 @@ describe('timeout options', () => { ); registerRouter(router); await server.start(); - await supertest(innerServer.listener).delete('/').expect(200, { - timeout: 300000, - }); + await supertest(innerServer.listener) + .delete('/') + .expect(200, { + timeout: { + payload: 300000, + idleSocket: 300001, + }, + }); }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a PUT', async () => { + test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a PUT', async () => { const { registerRouter, server: innerServer } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); @@ -1076,11 +1118,19 @@ describe('timeout options', () => { { path: '/', validate: false, - options: { timeout: 300000 }, + options: { + timeout: { + payload: 300000, + }, + }, }, (context, req, res) => { try { - return res.ok({ body: { timeout: req.route.options.timeout } }); + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); } catch (err) { return res.internalError({ body: err.message }); } @@ -1088,12 +1138,17 @@ describe('timeout options', () => { ); registerRouter(router); await server.start(); - await supertest(innerServer.listener).put('/').expect(200, { - timeout: 300000, - }); + await supertest(innerServer.listener) + .put('/') + .expect(200, { + timeout: { + payload: 300000, + idleSocket: 300001, + }, + }); }); - test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a PATCH', async () => { + test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a PATCH', async () => { const { registerRouter, server: innerServer } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); @@ -1101,11 +1156,19 @@ describe('timeout options', () => { { path: '/', validate: false, - options: { timeout: 300000 }, + options: { + timeout: { + payload: 300000, + }, + }, }, (context, req, res) => { try { - return res.ok({ body: { timeout: req.route.options.timeout } }); + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); } catch (err) { return res.internalError({ body: err.message }); } @@ -1113,9 +1176,14 @@ describe('timeout options', () => { ); registerRouter(router); await server.start(); - await supertest(innerServer.listener).patch('/').expect(200, { - timeout: 300000, - }); + await supertest(innerServer.listener) + .patch('/') + .expect(200, { + timeout: { + payload: 300000, + idleSocket: 300001, + }, + }); }); }); diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index 4b70f58deba99..ad731f25a1430 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -164,7 +164,10 @@ export class HttpServer { const { authRequired, tags, body = {}, timeout } = route.options; const { accepts: allow, maxBytes, output, parse } = body; // Hapi does not allow timeouts on payloads to be specified for 'head' or 'get' requests - const payloadTimeout = isSafeMethod(route.method) || timeout == null ? undefined : timeout; + const payloadTimeout = isSafeMethod(route.method) ? undefined : timeout?.payload; + + // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond + const socketTimeout = payloadTimeout ? payloadTimeout + 1 : undefined; const kibanaRouteState: KibanaRouteState = { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), @@ -194,12 +197,9 @@ export class HttpServer { timeout: payloadTimeout, } : undefined, - timeout: - timeout != null - ? { - socket: timeout + 1, // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond - } - : undefined, + timeout: { + socket: socketTimeout, + }, }, }); } diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 0e73431fe7c6d..f6909711621a6 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -196,15 +196,18 @@ export class KibanaRequest< private getRouteInfo(request: Request): KibanaRequestRoute { const method = request.method as Method; - const { parse, maxBytes, allow, output } = request.route.settings.payload || {}; - const timeout = request.route.settings.timeout?.socket; + const { parse, maxBytes, allow, output, timeout: payloadTimeout } = + request.route.settings.payload || {}; const options = ({ authRequired: this.getAuthRequired(request), // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 xsrfRequired: (request.route.settings.app as KibanaRouteState)?.xsrfRequired ?? true, tags: request.route.settings.tags || [], - timeout: typeof timeout === 'number' ? timeout - 1 : undefined, // We are forced to have the timeout be 1 millisecond greater than the server and payload so we subtract one here to give the user consist settings + timeout: { + payload: payloadTimeout, + idleSocket: request.route.settings.timeout.socket, + }, body: isSafeMethod(method) ? undefined : { diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index 676c494bec522..d4c3170b23441 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -146,10 +146,14 @@ export interface RouteConfigOptions { body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody; /** - * Timeouts for processing durations. Response timeout is in milliseconds. - * Default value: 2 minutes + * Defines per-route timeouts. */ - timeout?: number; + timeout?: { + /** + * Milliseconds to receive the payload + */ + payload?: number; + }; } /** diff --git a/x-pack/plugins/lists/server/routes/import_list_item_route.ts b/x-pack/plugins/lists/server/routes/import_list_item_route.ts index e162e7829e456..a5e813c7a49a9 100644 --- a/x-pack/plugins/lists/server/routes/import_list_item_route.ts +++ b/x-pack/plugins/lists/server/routes/import_list_item_route.ts @@ -27,7 +27,9 @@ export const importListItemRoute = (router: IRouter, config: ConfigType): void = parse: false, }, tags: ['access:lists-all'], - timeout: config.importTimeout.asMilliseconds(), + timeout: { + payload: config.importTimeout.asMilliseconds(), + }, }, path: `${LIST_ITEM_URL}/_import`, validate: { From 4e3b566c8286e328a556cdedfd17c701b2f1a092 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 29 Jul 2020 10:41:38 -0700 Subject: [PATCH 02/26] timeout.idleSocket can now be specified per route --- src/core/server/http/http_server.test.ts | 403 ++++++++++++++--------- src/core/server/http/http_server.ts | 6 +- src/core/server/http/router/route.ts | 5 + 3 files changed, 260 insertions(+), 154 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 2348dd789482d..1ae2b1c59ada3 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -68,6 +68,7 @@ beforeEach(() => { port: 10002, ssl: { enabled: false }, compression: { enabled: true }, + socketTimeout: 2000, } as HttpConfig; configWithSSL = { @@ -997,193 +998,289 @@ describe('body options', () => { }); describe('timeout options', () => { - test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a POST', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router('', logger, enhanceWithContext); - router.post( - { - path: '/', - validate: false, - options: { + describe('payload timeout', () => { + test('POST routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.post( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, + }, + }, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } + } + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .post('/') + .send({ test: 1 }) + .expect(200, { timeout: { payload: 300000, + idleSocket: 300001, }, - }, - }, - (context, req, res) => { - try { - return res.ok({ - body: { - timeout: req.route.options.timeout, + }); + }); + + test('GET routes ignore the specified payload timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.get( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, }, - }); - } catch (err) { - return res.internalError({ body: err.message }); - } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener) - .post('/') - .send({ test: 1 }) - .expect(200, { - timeout: { - payload: 300000, - idleSocket: 300001, + }, }, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } + } + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener).get('/').expect(200, { + timeout: {}, }); - }); - - test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a GET', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + }); - const router = new Router('', logger, enhanceWithContext); - router.get( - { - path: '/', - validate: false, - options: { - timeout: { - payload: 300000, + test('DELETE routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.delete( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, }, }, - }, - (context, req, res) => { - try { - return res.ok({ - body: { - timeout: req.route.options.timeout, - }, - }); - } catch (err) { - return res.internalError({ body: err.message }); + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: { + payload: 300000, + idleSocket: 300001, + }, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener).get('/').expect(200, { - timeout: {}, + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .delete('/') + .expect(200, { + timeout: { + payload: 300000, + idleSocket: 300001, + }, + }); }); - }); - test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a DELETE', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router('', logger, enhanceWithContext); - router.delete( - { - path: '/', - validate: false, - options: { + test('PUT routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.put( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, + }, + }, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); + } catch (err) { + return res.internalError({ body: err.message }); + } + } + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .put('/') + .expect(200, { timeout: { payload: 300000, + idleSocket: 300001, + }, + }); + }); + + test('PATCH routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.patch( + { + path: '/', + validate: false, + options: { + timeout: { + payload: 300000, + }, }, }, - }, - (context, req, res) => { - try { - return res.ok({ - body: { - timeout: { - payload: 300000, - idleSocket: 300001, + (context, req, res) => { + try { + return res.ok({ + body: { + timeout: req.route.options.timeout, }, - }, - }); - } catch (err) { - return res.internalError({ body: err.message }); + }); + } catch (err) { + return res.internalError({ body: err.message }); + } } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener) - .delete('/') - .expect(200, { - timeout: { - payload: 300000, - idleSocket: 300001, - }, - }); - }); - - test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a PUT', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router('', logger, enhanceWithContext); - router.put( - { - path: '/', - validate: false, - options: { + ); + registerRouter(router); + await server.start(); + await supertest(innerServer.listener) + .patch('/') + .expect(200, { timeout: { payload: 300000, + idleSocket: 300001, }, + }); + }); + }); + + describe('idleSocket timeout', () => { + test('sets the socket timeout when specified in the route', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.get( + { + path: '/', + validate: { body: schema.any() }, + options: { timeout: { idleSocket: 12000 } }, }, - }, - (context, req, res) => { - try { + (context, req, res) => { return res.ok({ body: { timeout: req.route.options.timeout, }, }); - } catch (err) { - return res.internalError({ body: err.message }); } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener) - .put('/') - .expect(200, { - timeout: { - payload: 300000, - idleSocket: 300001, - }, - }); - }); + ); + registerRouter(router); - test('should accept a payload timeout which is 3 minutes in milliseconds, "300000" for a PATCH', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router('', logger, enhanceWithContext); - router.patch( - { - path: '/', - validate: false, - options: { + await server.start(); + await supertest(innerServer.listener) + .get('/') + .send() + .expect(200, { timeout: { - payload: 300000, + idleSocket: 12000, }, - }, - }, - (context, req, res) => { - try { - return res.ok({ - body: { - timeout: req.route.options.timeout, + }); + }); + + test(`payload timeout doesn't cause the specified idleSocket timeout to be overwritten`, async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.post( + { + path: '/', + validate: { body: schema.any() }, + options: { + timeout: { + payload: 11000, + idleSocket: 12000, }, - }); - } catch (err) { - return res.internalError({ body: err.message }); + }, + }, + (context, req, res) => { + return res.ok({ body: { timeout: req.route.options.timeout } }); } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener) - .patch('/') - .expect(200, { - timeout: { - payload: 300000, - idleSocket: 300001, + ); + registerRouter(router); + + await server.start(); + await supertest(innerServer.listener) + .post('/') + .send({}) + .expect(200, { + timeout: { + payload: 11000, + idleSocket: 12000, + }, + }); + }); + + // this can ideally be deleted in the future once https://github.com/hapijs/hapi/issues/4122 is resolved + test(`idleSocket timeout must be larger than the payload timeout`, async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.post( + { + path: '/', + validate: { body: schema.any() }, + options: { + timeout: { + payload: 13000, + idleSocket: 12000, + }, + }, }, - }); + (context, req, res) => { + return res.ok({ body: { timeout: req.route.options.timeout } }); + } + ); + + registerRouter(router); + + await expect(() => server.start()).rejects.toThrowErrorMatchingInlineSnapshot( + `"Payload timeout must be shorter than socket timeout: post /"` + ); + }); }); }); diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index ad731f25a1430..b289a895af1d0 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -167,7 +167,11 @@ export class HttpServer { const payloadTimeout = isSafeMethod(route.method) ? undefined : timeout?.payload; // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond - const socketTimeout = payloadTimeout ? payloadTimeout + 1 : undefined; + const socketTimeout = timeout?.idleSocket + ? timeout.idleSocket + : payloadTimeout + ? payloadTimeout + 1 + : undefined; const kibanaRouteState: KibanaRouteState = { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index d4c3170b23441..ae6e49cd37503 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -153,6 +153,11 @@ export interface RouteConfigOptions { * Milliseconds to receive the payload */ payload?: number; + + /** + * Milliseconds the socket can be idle before it's closed + */ + idleSocket?: number; }; } From 26541badcc71c0c32e2d16f1754b5e763daccc54 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 29 Jul 2020 10:44:25 -0700 Subject: [PATCH 03/26] Removing nested ternary --- src/core/server/http/http_server.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index b289a895af1d0..ae17fd32cb7c2 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -166,12 +166,14 @@ export class HttpServer { // Hapi does not allow timeouts on payloads to be specified for 'head' or 'get' requests const payloadTimeout = isSafeMethod(route.method) ? undefined : timeout?.payload; - // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond - const socketTimeout = timeout?.idleSocket - ? timeout.idleSocket - : payloadTimeout - ? payloadTimeout + 1 - : undefined; + let socketTimeout: number | undefined; + if (timeout?.idleSocket) { + socketTimeout = timeout.idleSocket; + } else if (payloadTimeout) { + // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond + // and hide this complexity from the user. + socketTimeout = payloadTimeout + 1; + } const kibanaRouteState: KibanaRouteState = { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), From 4048150c27619e50e0f939765103d70d0fed102f Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 29 Jul 2020 10:50:43 -0700 Subject: [PATCH 04/26] Fixing integration tests --- .../http/integration_tests/router.test.ts | 206 +++++++++--------- 1 file changed, 104 insertions(+), 102 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 434e22e3cf6f5..4b6c5cc5345f8 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -304,126 +304,128 @@ describe('Options', () => { }); describe('timeout', () => { - it('should timeout if configured with a small timeout value for a POST', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + describe('payload', () => { + it('should timeout if configured with a small timeout value for a POST', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.post( - { path: '/a', validate: false, options: { timeout: 1000 } }, - async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); - return res.ok({}); - } - ); - router.post({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); - expect(supertest(innerServer.listener).post('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).post('/b').expect(200, {}); - }); + router.post( + { path: '/a', validate: false, options: { timeout: { payload: 1000 } } }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 2000)); + return res.ok({}); + } + ); + router.post({ path: '/b', validate: false }, (context, req, res) => res.ok({})); + await server.start(); + expect(supertest(innerServer.listener).post('/a')).rejects.toThrow('socket hang up'); + await supertest(innerServer.listener).post('/b').expect(200, {}); + }); - it('should timeout if configured with a small timeout value for a PUT', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + it('should timeout if configured with a small timeout value for a PUT', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.put( - { path: '/a', validate: false, options: { timeout: 1000 } }, - async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); - return res.ok({}); - } - ); - router.put({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); + router.put( + { path: '/a', validate: false, options: { timeout: { payload: 1000 } } }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 2000)); + return res.ok({}); + } + ); + router.put({ path: '/b', validate: false }, (context, req, res) => res.ok({})); + await server.start(); - expect(supertest(innerServer.listener).put('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).put('/b').expect(200, {}); - }); + expect(supertest(innerServer.listener).put('/a')).rejects.toThrow('socket hang up'); + await supertest(innerServer.listener).put('/b').expect(200, {}); + }); - it('should timeout if configured with a small timeout value for a DELETE', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + it('should timeout if configured with a small timeout value for a DELETE', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.delete( - { path: '/a', validate: false, options: { timeout: 1000 } }, - async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); - return res.ok({}); - } - ); - router.delete({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); - expect(supertest(innerServer.listener).delete('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).delete('/b').expect(200, {}); - }); + router.delete( + { path: '/a', validate: false, options: { timeout: { payload: 1000 } } }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 2000)); + return res.ok({}); + } + ); + router.delete({ path: '/b', validate: false }, (context, req, res) => res.ok({})); + await server.start(); + expect(supertest(innerServer.listener).delete('/a')).rejects.toThrow('socket hang up'); + await supertest(innerServer.listener).delete('/b').expect(200, {}); + }); - it('should timeout if configured with a small timeout value for a GET', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + it('should timeout if configured with a small timeout value for a GET', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.get( - // Note: There is a bug within Hapi Server where it cannot set the payload timeout for a GET call but it also cannot configure a timeout less than the payload body - // so the least amount of possible time to configure the timeout is 10 seconds. - { path: '/a', validate: false, options: { timeout: 100000 } }, - async (context, req, res) => { - // Cause a wait of 20 seconds to cause the socket hangup - await new Promise((resolve) => setTimeout(resolve, 200000)); - return res.ok({}); - } - ); - router.get({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); + router.get( + // Note: There is a bug within Hapi Server where it cannot set the payload timeout for a GET call but it also cannot configure a timeout less than the payload body + // so the least amount of possible time to configure the timeout is 10 seconds. + { path: '/a', validate: false, options: { timeout: { payload: 100000 } } }, + async (context, req, res) => { + // Cause a wait of 20 seconds to cause the socket hangup + await new Promise((resolve) => setTimeout(resolve, 200000)); + return res.ok({}); + } + ); + router.get({ path: '/b', validate: false }, (context, req, res) => res.ok({})); + await server.start(); - expect(supertest(innerServer.listener).get('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).get('/b').expect(200, {}); - }); + expect(supertest(innerServer.listener).get('/a')).rejects.toThrow('socket hang up'); + await supertest(innerServer.listener).get('/b').expect(200, {}); + }); - it('should not timeout if configured with a 5 minute timeout value for a POST', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + it('should not timeout if configured with a 5 minute timeout value for a POST', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.post( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - await supertest(innerServer.listener).post('/a').expect(200, {}); - }); + router.post( + { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, + async (context, req, res) => res.ok({}) + ); + await server.start(); + await supertest(innerServer.listener).post('/a').expect(200, {}); + }); - it('should not timeout if configured with a 5 minute timeout value for a PUT', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + it('should not timeout if configured with a 5 minute timeout value for a PUT', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.put( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); + router.put( + { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, + async (context, req, res) => res.ok({}) + ); + await server.start(); - await supertest(innerServer.listener).put('/a').expect(200, {}); - }); + await supertest(innerServer.listener).put('/a').expect(200, {}); + }); - it('should not timeout if configured with a 5 minute timeout value for a DELETE', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + it('should not timeout if configured with a 5 minute timeout value for a DELETE', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.delete( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - await supertest(innerServer.listener).delete('/a').expect(200, {}); - }); + router.delete( + { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, + async (context, req, res) => res.ok({}) + ); + await server.start(); + await supertest(innerServer.listener).delete('/a').expect(200, {}); + }); - it('should not timeout if configured with a 5 minute timeout value for a GET', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); + it('should not timeout if configured with a 5 minute timeout value for a GET', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); - router.get( - { path: '/a', validate: false, options: { timeout: 300000 } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - await supertest(innerServer.listener).get('/a').expect(200, {}); + router.get( + { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, + async (context, req, res) => res.ok({}) + ); + await server.start(); + await supertest(innerServer.listener).get('/a').expect(200, {}); + }); }); }); }); From 8267a2cb67853b58404011529f6fbcffcbfbe047 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 29 Jul 2020 12:16:42 -0700 Subject: [PATCH 05/26] Trying to actually fix the integration tests. Existing tests are hitting idle socket timeout, not the payload timeout --- .../http/integration_tests/router.test.ts | 48 ++++++++++++++++--- 1 file changed, 41 insertions(+), 7 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 4b6c5cc5345f8..b83dbca1ec242 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { Stream } from 'stream'; +import { Stream, PassThrough } from 'stream'; import Boom from 'boom'; import supertest from 'supertest'; import { schema } from '@kbn/config-schema'; @@ -305,21 +305,55 @@ describe('Options', () => { describe('timeout', () => { describe('payload', () => { - it('should timeout if configured with a small timeout value for a POST', async () => { + it.only('should timeout if payload takes too long to send', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); + innerServer.listener.on('request', () => { + console.log('REQUEST STARTED'); + }); + router.post( - { path: '/a', validate: false, options: { timeout: { payload: 1000 } } }, + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 1000, idleSocket: 120000 }, + }, + path: '/a', + validate: false, + }, async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); return res.ok({}); } ); - router.post({ path: '/b', validate: false }, (context, req, res) => res.ok({})); + // router.post({ path: '/b', validate: false }, (context, req, res) => res.ok({})); await server.start(); - expect(supertest(innerServer.listener).post('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).post('/b').expect(200, {}); + + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json'); + + const response = new Promise((resolve, reject) => { + request.on('error', (err) => { + reject(err); + }); + + request.on('response', (res) => { + resolve(res); + }); + }); + + // start writing some JSON, and just stop + const stream = new PassThrough(); + stream.pipe(request); + stream.write('{ '); + + // we shouldn't get a socket timeout, if we do it's because the socket timeout is hit + await expect(response).rejects.toThrowErrorMatchingInlineSnapshot(); + // await supertest(innerServer.listener).post('/b').expect(200, {}); }); it('should timeout if configured with a small timeout value for a PUT', async () => { From 66cf715479233822d1842675df5d530d08ca08f8 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 08:58:59 -0700 Subject: [PATCH 06/26] Fixing payload post timeout integration test --- .../http/integration_tests/router.test.ts | 46 +++++++++---------- 1 file changed, 22 insertions(+), 24 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index b83dbca1ec242..f35f6c98becf7 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { Stream, PassThrough } from 'stream'; +import { Stream } from 'stream'; import Boom from 'boom'; import supertest from 'supertest'; import { schema } from '@kbn/config-schema'; @@ -305,21 +305,17 @@ describe('Options', () => { describe('timeout', () => { describe('payload', () => { - it.only('should timeout if payload takes too long to send', async () => { + it('should timeout if payload takes too long to POST', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); - innerServer.listener.on('request', () => { - console.log('REQUEST STARTED'); - }); - router.post( { options: { body: { accepts: ['application/json'], }, - timeout: { payload: 1000, idleSocket: 120000 }, + timeout: { payload: 100, idleSocket: 120000 }, }, path: '/a', validate: false, @@ -328,7 +324,6 @@ describe('Options', () => { return res.ok({}); } ); - // router.post({ path: '/b', validate: false }, (context, req, res) => res.ok({})); await server.start(); // start the request @@ -336,24 +331,27 @@ describe('Options', () => { .post('/a') .set('Content-Type', 'application/json'); - const response = new Promise((resolve, reject) => { - request.on('error', (err) => { - reject(err); - }); - - request.on('response', (res) => { - resolve(res); - }); + // write some JSON very slowly... + const result = new Promise((resolve, reject) => { + const body = '{"foo":"bar"}'; + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + if (err) { + reject(err); + } else { + resolve(res); + } + }); + } + }, 10); }); - // start writing some JSON, and just stop - const stream = new PassThrough(); - stream.pipe(request); - stream.write('{ '); - - // we shouldn't get a socket timeout, if we do it's because the socket timeout is hit - await expect(response).rejects.toThrowErrorMatchingInlineSnapshot(); - // await supertest(innerServer.listener).post('/b').expect(200, {}); + await expect(result).resolves.toHaveProperty('status', 408); }); it('should timeout if configured with a small timeout value for a PUT', async () => { From 1a26b103927941e7739a169850f7b2dea6a3979f Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 13:26:33 -0700 Subject: [PATCH 07/26] Fixing PUT and DELETE payload sending too long tests --- .../http/integration_tests/router.test.ts | 91 ++++++++++++++++--- 1 file changed, 78 insertions(+), 13 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index f35f6c98becf7..6a30bb8e4e628 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -305,7 +305,7 @@ describe('Options', () => { describe('timeout', () => { describe('payload', () => { - it('should timeout if payload takes too long to POST', async () => { + it('should timeout if POST payload sending takes too long', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -354,39 +354,104 @@ describe('Options', () => { await expect(result).resolves.toHaveProperty('status', 408); }); - it('should timeout if configured with a small timeout value for a PUT', async () => { + it('should timeout if PUT payload sending takes too long', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); router.put( - { path: '/a', validate: false, options: { timeout: { payload: 1000 } } }, + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 100, idleSocket: 120000 }, + }, + path: '/a', + validate: false, + }, async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); return res.ok({}); } ); - router.put({ path: '/b', validate: false }, (context, req, res) => res.ok({})); await server.start(); - expect(supertest(innerServer.listener).put('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).put('/b').expect(200, {}); + // start the request + const request = supertest(innerServer.listener) + .put('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + // write some JSON very slowly... + const result = new Promise((resolve, reject) => { + const body = '{"foo":"bar"}'; + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + if (err) { + reject(err); + } else { + resolve(res); + } + }); + } + }, 10); + }); + + await expect(result).resolves.toHaveProperty('status', 408); }); - it('should timeout if configured with a small timeout value for a DELETE', async () => { + it('should timeout if DELETE payload sending takes too long', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); router.delete( - { path: '/a', validate: false, options: { timeout: { payload: 1000 } } }, + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 100, idleSocket: 120000 }, + }, + path: '/a', + validate: false, + }, async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); return res.ok({}); } ); - router.delete({ path: '/b', validate: false }, (context, req, res) => res.ok({})); await server.start(); - expect(supertest(innerServer.listener).delete('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).delete('/b').expect(200, {}); + + // start the request + const request = supertest(innerServer.listener) + .delete('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + // write some JSON very slowly... + const result = new Promise((resolve, reject) => { + const body = '{"foo":"bar"}'; + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + if (err) { + reject(err); + } else { + resolve(res); + } + }); + } + }, 10); + }); + + await expect(result).resolves.toHaveProperty('status', 408); }); it('should timeout if configured with a small timeout value for a GET', async () => { From c03a0cee97eb6018e9d62211807151f6b40e5c37 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 13:30:18 -0700 Subject: [PATCH 08/26] Fixing type-script errors --- src/core/server/http/http_server.test.ts | 2 +- src/core/server/http/router/request.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 1ae2b1c59ada3..7cc9f0fdc8ee2 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -1256,7 +1256,7 @@ describe('timeout options', () => { // this can ideally be deleted in the future once https://github.com/hapijs/hapi/issues/4122 is resolved test(`idleSocket timeout must be larger than the payload timeout`, async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + const { registerRouter } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); router.post( diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 8c79610263f83..32de4fed20b6b 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -221,7 +221,7 @@ export class KibanaRequest< tags: request.route.settings.tags || [], timeout: { payload: payloadTimeout, - idleSocket: request.route.settings.timeout.socket, + idleSocket: request.route.settings.timeout?.socket, }, body: isSafeMethod(method) ? undefined From b5b702606442e0ecc02a61f8d5d2a7134c4e8588 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 13:49:31 -0700 Subject: [PATCH 09/26] GET routes can't specify the payload timeout, they can't accept payloads --- src/core/server/http/http_server.test.ts | 33 ------------------- .../http/integration_tests/router.test.ts | 33 ------------------- src/core/server/http/router/route.ts | 2 +- 3 files changed, 1 insertion(+), 67 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index 7cc9f0fdc8ee2..b731da76f89e5 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -1038,39 +1038,6 @@ describe('timeout options', () => { }); }); - test('GET routes ignore the specified payload timeout', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); - - const router = new Router('', logger, enhanceWithContext); - router.get( - { - path: '/', - validate: false, - options: { - timeout: { - payload: 300000, - }, - }, - }, - (context, req, res) => { - try { - return res.ok({ - body: { - timeout: req.route.options.timeout, - }, - }); - } catch (err) { - return res.internalError({ body: err.message }); - } - } - ); - registerRouter(router); - await server.start(); - await supertest(innerServer.listener).get('/').expect(200, { - timeout: {}, - }); - }); - test('DELETE routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { const { registerRouter, server: innerServer } = await server.setup(config); diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 6a30bb8e4e628..d0ab7b19a9893 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -454,27 +454,6 @@ describe('Options', () => { await expect(result).resolves.toHaveProperty('status', 408); }); - it('should timeout if configured with a small timeout value for a GET', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); - - router.get( - // Note: There is a bug within Hapi Server where it cannot set the payload timeout for a GET call but it also cannot configure a timeout less than the payload body - // so the least amount of possible time to configure the timeout is 10 seconds. - { path: '/a', validate: false, options: { timeout: { payload: 100000 } } }, - async (context, req, res) => { - // Cause a wait of 20 seconds to cause the socket hangup - await new Promise((resolve) => setTimeout(resolve, 200000)); - return res.ok({}); - } - ); - router.get({ path: '/b', validate: false }, (context, req, res) => res.ok({})); - await server.start(); - - expect(supertest(innerServer.listener).get('/a')).rejects.toThrow('socket hang up'); - await supertest(innerServer.listener).get('/b').expect(200, {}); - }); - it('should not timeout if configured with a 5 minute timeout value for a POST', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -511,18 +490,6 @@ describe('Options', () => { await server.start(); await supertest(innerServer.listener).delete('/a').expect(200, {}); }); - - it('should not timeout if configured with a 5 minute timeout value for a GET', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); - - router.get( - { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - await supertest(innerServer.listener).get('/a').expect(200, {}); - }); }); }); }); diff --git a/src/core/server/http/router/route.ts b/src/core/server/http/router/route.ts index ae6e49cd37503..ce898a34e6b2c 100644 --- a/src/core/server/http/router/route.ts +++ b/src/core/server/http/router/route.ts @@ -152,7 +152,7 @@ export interface RouteConfigOptions { /** * Milliseconds to receive the payload */ - payload?: number; + payload?: Method extends 'get' | 'options' ? undefined : number; /** * Milliseconds the socket can be idle before it's closed From 7429fea28688140330701007ef235e7b4bbbcbf5 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 14:01:53 -0700 Subject: [PATCH 10/26] Removing some redundancy in the tests --- .../http/integration_tests/router.test.ts | 97 ++++++------------- 1 file changed, 31 insertions(+), 66 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index d0ab7b19a9893..c305e46f9d5f4 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -305,7 +305,26 @@ describe('Options', () => { describe('timeout', () => { describe('payload', () => { - it('should timeout if POST payload sending takes too long', async () => { + const writeBodyCharAtATime = (request: supertest.Test, body: string, interval: number) => { + return new Promise((resolve, reject) => { + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + if (err) { + reject(err); + } else { + resolve(res); + } + }); + } + }, interval); + }); + }; + it('should timeout if POST payload sending is too slow', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -331,30 +350,12 @@ describe('Options', () => { .post('/a') .set('Content-Type', 'application/json'); - // write some JSON very slowly... - const result = new Promise((resolve, reject) => { - const body = '{"foo":"bar"}'; - let i = 0; - const intervalId = setInterval(() => { - if (i < body.length) { - request.write(body[i++]); - } else { - clearInterval(intervalId); - request.end((err, res) => { - if (err) { - reject(err); - } else { - resolve(res); - } - }); - } - }, 10); - }); + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); await expect(result).resolves.toHaveProperty('status', 408); }); - it('should timeout if PUT payload sending takes too long', async () => { + it('should timeout if PUT payload sending is too slow', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -381,30 +382,12 @@ describe('Options', () => { .set('Content-Type', 'application/json') .set('Transfer-Encoding', 'chunked'); - // write some JSON very slowly... - const result = new Promise((resolve, reject) => { - const body = '{"foo":"bar"}'; - let i = 0; - const intervalId = setInterval(() => { - if (i < body.length) { - request.write(body[i++]); - } else { - clearInterval(intervalId); - request.end((err, res) => { - if (err) { - reject(err); - } else { - resolve(res); - } - }); - } - }, 10); - }); + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); await expect(result).resolves.toHaveProperty('status', 408); }); - it('should timeout if DELETE payload sending takes too long', async () => { + it('should timeout if DELETE payload sending is too slow', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -431,47 +414,29 @@ describe('Options', () => { .set('Content-Type', 'application/json') .set('Transfer-Encoding', 'chunked'); - // write some JSON very slowly... - const result = new Promise((resolve, reject) => { - const body = '{"foo":"bar"}'; - let i = 0; - const intervalId = setInterval(() => { - if (i < body.length) { - request.write(body[i++]); - } else { - clearInterval(intervalId); - request.end((err, res) => { - if (err) { - reject(err); - } else { - resolve(res); - } - }); - } - }, 10); - }); + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); await expect(result).resolves.toHaveProperty('status', 408); }); - it('should not timeout if configured with a 5 minute timeout value for a POST', async () => { + it('should not timeout if POST payload sending is quick', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); router.post( - { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, + { path: '/a', validate: false, options: { timeout: { payload: 10000 } } }, async (context, req, res) => res.ok({}) ); await server.start(); await supertest(innerServer.listener).post('/a').expect(200, {}); }); - it('should not timeout if configured with a 5 minute timeout value for a PUT', async () => { + it('should not timeout if PUT payload sending is quick', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); router.put( - { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, + { path: '/a', validate: false, options: { timeout: { payload: 10000 } } }, async (context, req, res) => res.ok({}) ); await server.start(); @@ -479,12 +444,12 @@ describe('Options', () => { await supertest(innerServer.listener).put('/a').expect(200, {}); }); - it('should not timeout if configured with a 5 minute timeout value for a DELETE', async () => { + it('should not timeout if DELETE payload sending is quick', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); router.delete( - { path: '/a', validate: false, options: { timeout: { payload: 300000 } } }, + { path: '/a', validate: false, options: { timeout: { payload: 10000 } } }, async (context, req, res) => res.ok({}) ); await server.start(); From 4f7e9e1e20e1501d68edfd3337fbc450c05cc043 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 15:03:49 -0700 Subject: [PATCH 11/26] Adding 'Transfer-Encoding: chunked' to the POST test --- src/core/server/http/integration_tests/router.test.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index c305e46f9d5f4..331c3c1f4bd36 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -348,7 +348,8 @@ describe('Options', () => { // start the request const request = supertest(innerServer.listener) .post('/a') - .set('Content-Type', 'application/json'); + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); From 828eab88a23d76b02357125a4c752e8b0a99a801 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 15:10:42 -0700 Subject: [PATCH 12/26] Fixing POST/GET/PUT quick tests --- .../http/integration_tests/router.test.ts | 47 ++++++++++++++++--- 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 331c3c1f4bd36..7246f99d41006 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -425,11 +425,24 @@ describe('Options', () => { const router = createRouter('/'); router.post( - { path: '/a', validate: false, options: { timeout: { payload: 10000 } } }, + { + path: '/a', + validate: false, + options: { body: { accepts: 'application/json' }, timeout: { payload: 10000 } }, + }, async (context, req, res) => res.ok({}) ); await server.start(); - await supertest(innerServer.listener).post('/a').expect(200, {}); + + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + const result = writeBodyCharAtATime(request, '{}', 10); + + await expect(result).resolves.toHaveProperty('status', 200); }); it('should not timeout if PUT payload sending is quick', async () => { @@ -437,12 +450,23 @@ describe('Options', () => { const router = createRouter('/'); router.put( - { path: '/a', validate: false, options: { timeout: { payload: 10000 } } }, + { + path: '/a', + validate: false, + options: { body: { accepts: 'application/json' }, timeout: { payload: 10000 } }, + }, async (context, req, res) => res.ok({}) ); await server.start(); - await supertest(innerServer.listener).put('/a').expect(200, {}); + const request = supertest(innerServer.listener) + .put('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + const result = writeBodyCharAtATime(request, '{}', 10); + + await expect(result).resolves.toHaveProperty('status', 200); }); it('should not timeout if DELETE payload sending is quick', async () => { @@ -450,11 +474,22 @@ describe('Options', () => { const router = createRouter('/'); router.delete( - { path: '/a', validate: false, options: { timeout: { payload: 10000 } } }, + { + path: '/a', + validate: false, + options: { body: { accepts: 'application/json' }, timeout: { payload: 10000 } }, + }, async (context, req, res) => res.ok({}) ); await server.start(); - await supertest(innerServer.listener).delete('/a').expect(200, {}); + const request = supertest(innerServer.listener) + .delete('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + const result = writeBodyCharAtATime(request, '{}', 10); + + await expect(result).resolves.toHaveProperty('status', 200); }); }); }); From c092cbea832ed0efc80fe5b6f0253409908d2bb8 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 15:20:43 -0700 Subject: [PATCH 13/26] Adding idleSocket timeout test --- .../http/integration_tests/router.test.ts | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 7246f99d41006..bdde3d6ac5719 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -324,6 +324,7 @@ describe('Options', () => { }, interval); }); }; + it('should timeout if POST payload sending is too slow', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -492,6 +493,47 @@ describe('Options', () => { await expect(result).resolves.toHaveProperty('status', 200); }); }); + + describe('idleSocket', () => { + // HapiJS forces the idleSocket timeout to be larger than the payload timeout, so we can't slowly + // send the payload to trigger the idle socket... + // We can add this test once https://github.com/hapijs/hapi/issues/4122 is fixed + + it('should timeout if servers response is too slow', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); + + router.post( + { path: '/a', validate: false, options: { timeout: { idleSocket: 1000, payload: 100 } } }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 2000)); + return res.ok({}); + } + ); + + await server.start(); + await expect(supertest(innerServer.listener).post('/a')).rejects.toThrow('socket hang up'); + }); + + it('should not timeout if servers response is quick', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); + + router.post( + { path: '/a', validate: false, options: { timeout: { idleSocket: 2000, payload: 100 } } }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 10)); + return res.ok({}); + } + ); + + await server.start(); + await expect(supertest(innerServer.listener).post('/a')).resolves.toHaveProperty( + 'status', + 200 + ); + }); + }); }); }); From 2c939ea61d580d23d5df606c8b795f9f91380558 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 15:25:26 -0700 Subject: [PATCH 14/26] Removing unnecessary `isSafeMethod` call --- src/core/server/http/http_server.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index ae17fd32cb7c2..4ce3d0cf4f5a3 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -163,8 +163,8 @@ export class HttpServer { const validate = isSafeMethod(route.method) ? undefined : { payload: true }; const { authRequired, tags, body = {}, timeout } = route.options; const { accepts: allow, maxBytes, output, parse } = body; - // Hapi does not allow timeouts on payloads to be specified for 'head' or 'get' requests - const payloadTimeout = isSafeMethod(route.method) ? undefined : timeout?.payload; + // Hapi does not allow timeouts on payloads to be specified for 'head' or 'get' requests so neither do we + const payloadTimeout = timeout?.payload; let socketTimeout: number | undefined; if (timeout?.idleSocket) { From d300b9f65bb34218fda064b05efb418488188453 Mon Sep 17 00:00:00 2001 From: kobelb Date: Wed, 5 Aug 2020 16:04:05 -0700 Subject: [PATCH 15/26] Updating documentation --- .../server/kibana-plugin-core-server.routeconfigoptions.md | 2 +- ...kibana-plugin-core-server.routeconfigoptions.timeout.md | 7 +++++-- src/core/server/server.api.md | 5 ++++- 3 files changed, 10 insertions(+), 4 deletions(-) diff --git a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md index fee6124f8d866..17fafe2af0de1 100644 --- a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md +++ b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.md @@ -19,6 +19,6 @@ export interface RouteConfigOptions | [authRequired](./kibana-plugin-core-server.routeconfigoptions.authrequired.md) | boolean | 'optional' | Defines authentication mode for a route: - true. A user has to have valid credentials to access a resource - false. A user can access a resource without any credentials. - 'optional'. A user can access a resource if has valid credentials or no credentials at all. Can be useful when we grant access to a resource but want to identify a user if possible.Defaults to true if an auth mechanism is registered. | | [body](./kibana-plugin-core-server.routeconfigoptions.body.md) | Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody | Additional body options [RouteConfigOptionsBody](./kibana-plugin-core-server.routeconfigoptionsbody.md). | | [tags](./kibana-plugin-core-server.routeconfigoptions.tags.md) | readonly string[] | Additional metadata tag strings to attach to the route. | -| [timeout](./kibana-plugin-core-server.routeconfigoptions.timeout.md) | number | Timeouts for processing durations. Response timeout is in milliseconds. Default value: 2 minutes | +| [timeout](./kibana-plugin-core-server.routeconfigoptions.timeout.md) | {
payload?: Method extends 'get' | 'options' ? undefined : number;
idleSocket?: number;
} | Defines per-route timeouts. | | [xsrfRequired](./kibana-plugin-core-server.routeconfigoptions.xsrfrequired.md) | Method extends 'get' ? never : boolean | Defines xsrf protection requirements for a route: - true. Requires an incoming POST/PUT/DELETE request to contain kbn-xsrf header. - false. Disables xsrf protection.Set to true by default | diff --git a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md index 479fcf883ec4d..f602a8913964f 100644 --- a/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md +++ b/docs/development/core/server/kibana-plugin-core-server.routeconfigoptions.timeout.md @@ -4,10 +4,13 @@ ## RouteConfigOptions.timeout property -Timeouts for processing durations. Response timeout is in milliseconds. Default value: 2 minutes +Defines per-route timeouts. Signature: ```typescript -timeout?: number; +timeout?: { + payload?: Method extends 'get' | 'options' ? undefined : number; + idleSocket?: number; + }; ``` diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 21ef66230f698..9787cc212d90c 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1884,7 +1884,10 @@ export interface RouteConfigOptions { authRequired?: boolean | 'optional'; body?: Method extends 'get' | 'options' ? undefined : RouteConfigOptionsBody; tags?: readonly string[]; - timeout?: number; + timeout?: { + payload?: Method extends 'get' | 'options' ? undefined : number; + idleSocket?: number; + }; xsrfRequired?: Method extends 'get' ? never : boolean; } From 84e06fa61da7b2945f9a145003d7fd2ffe29fd41 Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 6 Aug 2020 06:44:01 -0700 Subject: [PATCH 16/26] Removing PUT/DELETE integration tests --- .../http/integration_tests/router.test.ts | 111 ------------------ 1 file changed, 111 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index bdde3d6ac5719..802864a9176d9 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -357,70 +357,6 @@ describe('Options', () => { await expect(result).resolves.toHaveProperty('status', 408); }); - it('should timeout if PUT payload sending is too slow', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); - - router.put( - { - options: { - body: { - accepts: ['application/json'], - }, - timeout: { payload: 100, idleSocket: 120000 }, - }, - path: '/a', - validate: false, - }, - async (context, req, res) => { - return res.ok({}); - } - ); - await server.start(); - - // start the request - const request = supertest(innerServer.listener) - .put('/a') - .set('Content-Type', 'application/json') - .set('Transfer-Encoding', 'chunked'); - - const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); - - await expect(result).resolves.toHaveProperty('status', 408); - }); - - it('should timeout if DELETE payload sending is too slow', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); - - router.delete( - { - options: { - body: { - accepts: ['application/json'], - }, - timeout: { payload: 100, idleSocket: 120000 }, - }, - path: '/a', - validate: false, - }, - async (context, req, res) => { - return res.ok({}); - } - ); - await server.start(); - - // start the request - const request = supertest(innerServer.listener) - .delete('/a') - .set('Content-Type', 'application/json') - .set('Transfer-Encoding', 'chunked'); - - const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); - - await expect(result).resolves.toHaveProperty('status', 408); - }); - it('should not timeout if POST payload sending is quick', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -445,53 +381,6 @@ describe('Options', () => { await expect(result).resolves.toHaveProperty('status', 200); }); - - it('should not timeout if PUT payload sending is quick', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); - - router.put( - { - path: '/a', - validate: false, - options: { body: { accepts: 'application/json' }, timeout: { payload: 10000 } }, - }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - - const request = supertest(innerServer.listener) - .put('/a') - .set('Content-Type', 'application/json') - .set('Transfer-Encoding', 'chunked'); - - const result = writeBodyCharAtATime(request, '{}', 10); - - await expect(result).resolves.toHaveProperty('status', 200); - }); - - it('should not timeout if DELETE payload sending is quick', async () => { - const { server: innerServer, createRouter } = await server.setup(setupDeps); - const router = createRouter('/'); - - router.delete( - { - path: '/a', - validate: false, - options: { body: { accepts: 'application/json' }, timeout: { payload: 10000 } }, - }, - async (context, req, res) => res.ok({}) - ); - await server.start(); - const request = supertest(innerServer.listener) - .delete('/a') - .set('Content-Type', 'application/json') - .set('Transfer-Encoding', 'chunked'); - - const result = writeBodyCharAtATime(request, '{}', 10); - - await expect(result).resolves.toHaveProperty('status', 200); - }); }); describe('idleSocket', () => { From 3d8901acab2c24c873b915793c9b4ce2b65bd922 Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 6 Aug 2020 09:59:06 -0700 Subject: [PATCH 17/26] Working around the HapiJS bug --- src/core/server/http/http_server.test.ts | 92 ++++++------ src/core/server/http/http_server.ts | 35 +++-- .../http/integration_tests/router.test.ts | 133 ++++++++++++++---- src/core/server/http/router/request.ts | 4 +- 4 files changed, 172 insertions(+), 92 deletions(-) diff --git a/src/core/server/http/http_server.test.ts b/src/core/server/http/http_server.test.ts index b731da76f89e5..abe70e003732b 100644 --- a/src/core/server/http/http_server.test.ts +++ b/src/core/server/http/http_server.test.ts @@ -68,7 +68,6 @@ beforeEach(() => { port: 10002, ssl: { enabled: false }, compression: { enabled: true }, - socketTimeout: 2000, } as HttpConfig; configWithSSL = { @@ -999,7 +998,7 @@ describe('body options', () => { describe('timeout options', () => { describe('payload timeout', () => { - test('POST routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + test('POST routes set the payload timeout', async () => { const { registerRouter, server: innerServer } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); @@ -1033,12 +1032,11 @@ describe('timeout options', () => { .expect(200, { timeout: { payload: 300000, - idleSocket: 300001, }, }); }); - test('DELETE routes set the payload timeout and automatically adjusts the idle socket timeout', async () => { + test('DELETE routes set the payload timeout', async () => { const { registerRouter, server: innerServer } = await server.setup(config); const router = new Router('', logger, enhanceWithContext); @@ -1056,10 +1054,7 @@ describe('timeout options', () => { try { return res.ok({ body: { - timeout: { - payload: 300000, - idleSocket: 300001, - }, + timeout: req.route.options.timeout, }, }); } catch (err) { @@ -1074,7 +1069,6 @@ describe('timeout options', () => { .expect(200, { timeout: { payload: 300000, - idleSocket: 300001, }, }); }); @@ -1112,7 +1106,6 @@ describe('timeout options', () => { .expect(200, { timeout: { payload: 300000, - idleSocket: 300001, }, }); }); @@ -1150,22 +1143,23 @@ describe('timeout options', () => { .expect(200, { timeout: { payload: 300000, - idleSocket: 300001, }, }); }); }); describe('idleSocket timeout', () => { - test('sets the socket timeout when specified in the route', async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + test('uses server socket timeout when not specified in the route', async () => { + const { registerRouter, server: innerServer } = await server.setup({ + ...config, + socketTimeout: 11000, + }); const router = new Router('', logger, enhanceWithContext); router.get( { path: '/', validate: { body: schema.any() }, - options: { timeout: { idleSocket: 12000 } }, }, (context, req, res) => { return res.ok({ @@ -1183,71 +1177,69 @@ describe('timeout options', () => { .send() .expect(200, { timeout: { - idleSocket: 12000, + idleSocket: 11000, }, }); }); - test(`payload timeout doesn't cause the specified idleSocket timeout to be overwritten`, async () => { - const { registerRouter, server: innerServer } = await server.setup(config); + test('sets the socket timeout when specified in the route', async () => { + const { registerRouter, server: innerServer } = await server.setup({ + ...config, + socketTimeout: 11000, + }); const router = new Router('', logger, enhanceWithContext); - router.post( + router.get( { path: '/', validate: { body: schema.any() }, - options: { - timeout: { - payload: 11000, - idleSocket: 12000, - }, - }, + options: { timeout: { idleSocket: 12000 } }, }, (context, req, res) => { - return res.ok({ body: { timeout: req.route.options.timeout } }); + return res.ok({ + body: { + timeout: req.route.options.timeout, + }, + }); } ); registerRouter(router); await server.start(); await supertest(innerServer.listener) - .post('/') - .send({}) + .get('/') + .send() .expect(200, { timeout: { - payload: 11000, idleSocket: 12000, }, }); }); + }); - // this can ideally be deleted in the future once https://github.com/hapijs/hapi/issues/4122 is resolved - test(`idleSocket timeout must be larger than the payload timeout`, async () => { - const { registerRouter } = await server.setup(config); + test(`idleSocket timeout can be smaller than the payload timeout`, async () => { + const { registerRouter } = await server.setup(config); - const router = new Router('', logger, enhanceWithContext); - router.post( - { - path: '/', - validate: { body: schema.any() }, - options: { - timeout: { - payload: 13000, - idleSocket: 12000, - }, + const router = new Router('', logger, enhanceWithContext); + router.post( + { + path: '/', + validate: { body: schema.any() }, + options: { + timeout: { + payload: 1000, + idleSocket: 10, }, }, - (context, req, res) => { - return res.ok({ body: { timeout: req.route.options.timeout } }); - } - ); + }, + (context, req, res) => { + return res.ok({ body: { timeout: req.route.options.timeout } }); + } + ); - registerRouter(router); + registerRouter(router); - await expect(() => server.start()).rejects.toThrowErrorMatchingInlineSnapshot( - `"Payload timeout must be shorter than socket timeout: post /"` - ); - }); + await server.start(); }); }); diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index 4ce3d0cf4f5a3..ddf6cf281a43f 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { Server } from 'hapi'; +import { Request, Server } from 'hapi'; import HapiStaticFiles from 'inert'; import url from 'url'; @@ -163,22 +163,14 @@ export class HttpServer { const validate = isSafeMethod(route.method) ? undefined : { payload: true }; const { authRequired, tags, body = {}, timeout } = route.options; const { accepts: allow, maxBytes, output, parse } = body; - // Hapi does not allow timeouts on payloads to be specified for 'head' or 'get' requests so neither do we - const payloadTimeout = timeout?.payload; - - let socketTimeout: number | undefined; - if (timeout?.idleSocket) { - socketTimeout = timeout.idleSocket; - } else if (payloadTimeout) { - // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond - // and hide this complexity from the user. - socketTimeout = payloadTimeout + 1; - } const kibanaRouteState: KibanaRouteState = { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), }; + // Works around https://github.com/hapijs/hapi/issues/4122 until v20 + const fakeSocketTimeout = timeout?.payload ? timeout.payload + 1 : undefined; + this.server.route({ handler: route.handler, method: route.method, @@ -186,13 +178,26 @@ export class HttpServer { options: { auth: this.getAuthOption(authRequired), app: kibanaRouteState, + ext: { + onPreAuth: { + method: (request, reply) => { + if (timeout?.idleSocket) { + request.raw.req.socket.setTimeout(timeout?.idleSocket); + } else if (fakeSocketTimeout) { + request.raw.req.socket.setTimeout(this.config!.socketTimeout || 0); + } + + return reply.continue; + }, + }, + }, tags: tags ? Array.from(tags) : undefined, // TODO: This 'validate' section can be removed once the legacy platform is completely removed. // We are telling Hapi that NP routes can accept any payload, so that it can bypass the default // validation applied in ./http_tools#getServerOptions // (All NP routes are already required to specify their own validation in order to access the payload) validate, - payload: [allow, maxBytes, output, parse, payloadTimeout].some( + payload: [allow, maxBytes, output, parse, timeout?.payload].some( (v) => typeof v !== 'undefined' ) ? { @@ -200,11 +205,11 @@ export class HttpServer { maxBytes, output, parse, - timeout: payloadTimeout, + timeout: timeout?.payload, } : undefined, timeout: { - socket: socketTimeout, + socket: fakeSocketTimeout, }, }, }); diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 802864a9176d9..40747aa3581a5 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -304,27 +304,27 @@ describe('Options', () => { }); describe('timeout', () => { - describe('payload', () => { - const writeBodyCharAtATime = (request: supertest.Test, body: string, interval: number) => { - return new Promise((resolve, reject) => { - let i = 0; - const intervalId = setInterval(() => { - if (i < body.length) { - request.write(body[i++]); - } else { - clearInterval(intervalId); - request.end((err, res) => { - if (err) { - reject(err); - } else { - resolve(res); - } - }); - } - }, interval); + const writeBodyCharAtATime = (request: supertest.Test, body: string, interval: number) => { + return new Promise((resolve, reject) => { + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + resolve(res); + }); + } + }, interval); + request.on('error', (err) => { + clearInterval(intervalId); + reject(err); }); - }; + }); + }; + describe('payload', () => { it('should timeout if POST payload sending is too slow', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); @@ -335,7 +335,7 @@ describe('Options', () => { body: { accepts: ['application/json'], }, - timeout: { payload: 100, idleSocket: 120000 }, + timeout: { payload: 100 }, }, path: '/a', validate: false, @@ -354,7 +354,7 @@ describe('Options', () => { const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); - await expect(result).resolves.toHaveProperty('status', 408); + await expect(result).rejects.toMatchInlineSnapshot(`[Error: Request Timeout]`); }); it('should not timeout if POST payload sending is quick', async () => { @@ -384,16 +384,88 @@ describe('Options', () => { }); describe('idleSocket', () => { - // HapiJS forces the idleSocket timeout to be larger than the payload timeout, so we can't slowly - // send the payload to trigger the idle socket... - // We can add this test once https://github.com/hapijs/hapi/issues/4122 is fixed + it('should timeout if payload sending has too long of an idle period', async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); + + router.post( + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 10 }, + }, + }, + async (context, req, res) => { + await new Promise((resolve) => setTimeout(resolve, 2000)); + return res.ok({}); + } + ); + + await server.start(); + + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + const result = writeBodyCharAtATime(request, '{}', 20); + + await expect(result).rejects.toThrow('socket hang up'); + }); + + it(`should not timeout if payload sending doesn't have too long of an idle period`, async () => { + const { server: innerServer, createRouter } = await server.setup(setupDeps); + const router = createRouter('/'); + + router.post( + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 1000 }, + }, + }, + async (context, req, res) => { + return res.ok({}); + } + ); + + await server.start(); + + // start the request + const request = supertest(innerServer.listener) + .post('/a') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked'); + + const result = writeBodyCharAtATime(request, '{}', 10); + + await expect(result).resolves.toHaveProperty('status', 200); + }); it('should timeout if servers response is too slow', async () => { const { server: innerServer, createRouter } = await server.setup(setupDeps); const router = createRouter('/'); router.post( - { path: '/a', validate: false, options: { timeout: { idleSocket: 1000, payload: 100 } } }, + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 1000, payload: 100 }, + }, + }, async (context, req, res) => { await new Promise((resolve) => setTimeout(resolve, 2000)); return res.ok({}); @@ -409,7 +481,16 @@ describe('Options', () => { const router = createRouter('/'); router.post( - { path: '/a', validate: false, options: { timeout: { idleSocket: 2000, payload: 100 } } }, + { + path: '/a', + validate: false, + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 2000, payload: 100 }, + }, + }, async (context, req, res) => { await new Promise((resolve) => setTimeout(resolve, 10)); return res.ok({}); diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index 32de4fed20b6b..c299d722d7f0c 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -214,6 +214,8 @@ export class KibanaRequest< const { parse, maxBytes, allow, output, timeout: payloadTimeout } = request.route.settings.payload || {}; + // net.Socket#timeout isn't documented, yet, and isn't part of the types, yet... https://github.com/nodejs/node/pull/34543 + const socketTimeout = (request.raw.req.socket as any).timeout; const options = ({ authRequired: this.getAuthRequired(request), // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 @@ -221,7 +223,7 @@ export class KibanaRequest< tags: request.route.settings.tags || [], timeout: { payload: payloadTimeout, - idleSocket: request.route.settings.timeout?.socket, + idleSocket: socketTimeout === 0 ? undefined : socketTimeout, }, body: isSafeMethod(method) ? undefined From afbb7e8c3fc939ef7aa803ba5b7d900842d3486b Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 6 Aug 2020 10:12:50 -0700 Subject: [PATCH 18/26] Deleting unused type import --- src/core/server/http/http_server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index ddf6cf281a43f..ceb01f6dd71d6 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import { Request, Server } from 'hapi'; +import { Server } from 'hapi'; import HapiStaticFiles from 'inert'; import url from 'url'; From c4a3e153fb5e8ee72e4a9b76ead0e504c596f7ea Mon Sep 17 00:00:00 2001 From: kobelb Date: Thu, 6 Aug 2020 12:19:32 -0700 Subject: [PATCH 19/26] The socket can be undefined... This occurs when using @hapi/shot directly or indirectly via Server.inject. In these scenarios, there isn't a socket. This can also occur when a "fake request" is used by the hacky background jobs: Reporting and Alerting... --- src/core/server/http/router/request.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/server/http/router/request.ts b/src/core/server/http/router/request.ts index c299d722d7f0c..278bc222b754b 100644 --- a/src/core/server/http/router/request.ts +++ b/src/core/server/http/router/request.ts @@ -214,8 +214,9 @@ export class KibanaRequest< const { parse, maxBytes, allow, output, timeout: payloadTimeout } = request.route.settings.payload || {}; - // net.Socket#timeout isn't documented, yet, and isn't part of the types, yet... https://github.com/nodejs/node/pull/34543 - const socketTimeout = (request.raw.req.socket as any).timeout; + // net.Socket#timeout isn't documented, yet, and isn't part of the types... https://github.com/nodejs/node/pull/34543 + // the socket is also undefined when using @hapi/shot, or when a "fake request" is used + const socketTimeout = (request.raw.req.socket as any)?.timeout; const options = ({ authRequired: this.getAuthRequired(request), // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 From 20ff93fcc072d379a433f448124c393a9cfb6a96 Mon Sep 17 00:00:00 2001 From: Brandon Kobel Date: Mon, 10 Aug 2020 12:59:40 -0700 Subject: [PATCH 20/26] Update src/core/server/http/http_server.ts Co-authored-by: Josh Dover --- src/core/server/http/http_server.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index ceb01f6dd71d6..1cc15f806a166 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -180,14 +180,14 @@ export class HttpServer { app: kibanaRouteState, ext: { onPreAuth: { - method: (request, reply) => { + method: (request, h) => { if (timeout?.idleSocket) { request.raw.req.socket.setTimeout(timeout?.idleSocket); } else if (fakeSocketTimeout) { request.raw.req.socket.setTimeout(this.config!.socketTimeout || 0); } - return reply.continue; + return h.continue; }, }, }, From dc430b9e42101f25ec3b88f8cdec7967de7ed8ad Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 10 Aug 2020 15:48:12 -0700 Subject: [PATCH 21/26] Adding payload timeout functional tests --- test/plugin_functional/config.ts | 1 + .../core_plugin_route_timeouts/kibana.json | 8 ++ .../core_plugin_route_timeouts/package.json | 17 ++++ .../server/index.ts | 23 +++++ .../server/plugin.ts | 73 +++++++++++++++ .../core_plugin_route_timeouts/tsconfig.json | 14 +++ .../test_suites/core/index.ts | 25 +++++ .../test_suites/core/route.ts | 91 +++++++++++++++++++ 8 files changed, 252 insertions(+) create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/package.json create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts create mode 100644 test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json create mode 100644 test/plugin_functional/test_suites/core/index.ts create mode 100644 test/plugin_functional/test_suites/core/route.ts diff --git a/test/plugin_functional/config.ts b/test/plugin_functional/config.ts index c611300eade10..9ec82e291e537 100644 --- a/test/plugin_functional/config.ts +++ b/test/plugin_functional/config.ts @@ -31,6 +31,7 @@ export default async function ({ readConfigFile }: FtrConfigProviderContext) { return { testFiles: [ + require.resolve('./test_suites/core'), require.resolve('./test_suites/custom_visualizations'), require.resolve('./test_suites/panel_actions'), require.resolve('./test_suites/core_plugins'), diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json new file mode 100644 index 0000000000000..6fbddad22b764 --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/kibana.json @@ -0,0 +1,8 @@ +{ + "id": "core_plugin_route_timeouts", + "version": "0.0.1", + "kibanaVersion": "kibana", + "configPath": ["core_plugin_route_timeouts"], + "server": true, + "ui": false +} diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/package.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/package.json new file mode 100644 index 0000000000000..a9c520338457b --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/package.json @@ -0,0 +1,17 @@ +{ + "name": "core_plugin_route_timeouts", + "version": "1.0.0", + "main": "target/test/plugin_functional/plugins/core_plugin_route_timeouts", + "kibana": { + "version": "kibana", + "templateVersion": "1.0.0" + }, + "license": "Apache-2.0", + "scripts": { + "kbn": "node ../../../../scripts/kbn.js", + "build": "rm -rf './target' && tsc" + }, + "devDependencies": { + "typescript": "3.9.5" + } +} diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts new file mode 100644 index 0000000000000..4fdd6ef0ab04a --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/index.ts @@ -0,0 +1,23 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { CorePluginRouteTimeoutsPlugin } from './plugin'; +export { PluginARequestContext } from './plugin'; + +export const plugin = () => new CorePluginRouteTimeoutsPlugin(); diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts new file mode 100644 index 0000000000000..10ffffcd6d41c --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts @@ -0,0 +1,73 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { Plugin, CoreSetup } from 'kibana/server'; + +export interface PluginARequestContext { + ping: () => Promise; +} + +declare module 'kibana/server' { + interface RequestHandlerContext { + pluginA?: PluginARequestContext; + } +} + +export class CorePluginRouteTimeoutsPlugin implements Plugin { + public setup(core: CoreSetup, deps: {}) { + const { http } = core; + + const router = http.createRouter(); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 100 }, + }, + path: '/short_payload_timeout', + validate: false, + }, + async (context, req, res) => { + return res.ok({}); + } + ); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { payload: 10000 }, + }, + path: '/longer_payload_timeout', + validate: false, + }, + async (context, req, res) => { + return res.ok({}); + } + ); + } + + public start() {} + public stop() {} +} diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json new file mode 100644 index 0000000000000..4a564ee1e5578 --- /dev/null +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json @@ -0,0 +1,14 @@ +{ + "extends": "../../../../tsconfig.json", + "compilerOptions": { + "outDir": "./target", + "skipLibCheck": true + }, + "include": [ + "index.ts", + "public/**/*.ts", + "public/**/*.tsx", + "../../../../typings/**/*" + ], + "exclude": [] +} diff --git a/test/plugin_functional/test_suites/core/index.ts b/test/plugin_functional/test_suites/core/index.ts new file mode 100644 index 0000000000000..5852e21fc3b39 --- /dev/null +++ b/test/plugin_functional/test_suites/core/index.ts @@ -0,0 +1,25 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { PluginFunctionalProviderContext } from '../../services'; + +export default function ({ loadTestFile }: PluginFunctionalProviderContext) { + describe('core', function () { + loadTestFile(require.resolve('./route')); + }); +} diff --git a/test/plugin_functional/test_suites/core/route.ts b/test/plugin_functional/test_suites/core/route.ts new file mode 100644 index 0000000000000..20daf84cdf0cc --- /dev/null +++ b/test/plugin_functional/test_suites/core/route.ts @@ -0,0 +1,91 @@ +/* + * Licensed to Elasticsearch B.V. under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch B.V. licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import expect from '@kbn/expect'; +import { Test } from 'supertest'; +import { PluginFunctionalProviderContext } from '../../services'; + +export default function ({ getService }: PluginFunctionalProviderContext) { + const supertest = getService('supertest'); + + describe('route', function () { + describe('timeouts', function () { + describe('payload', function () { + const writeBodyCharAtATime = (request: Test, body: string, interval: number) => { + return new Promise((resolve, reject) => { + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { + clearInterval(intervalId); + request.end((err, res) => { + resolve(res); + }); + } + }, interval); + request.on('error', (err) => { + clearInterval(intervalId); + reject(err); + }); + }); + }; + it(`should timeout if POST payload sending is too slow`, async () => { + // start the request + const request = supertest + .post('/short_payload_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); + + await result.then( + (res) => { + expect(res).to.be(undefined); + }, + (err) => { + expect(err.message).to.be('Request Timeout'); + } + ); + }); + + it(`should not timeout if POST payload sending is quick`, async () => { + // start the request + const request = supertest + .post('/longer_payload_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"foo":"bar"}', 10); + + await result.then( + (res) => { + expect(res).to.have.property('statusCode', 200); + }, + (err) => { + expect(err).to.be(undefined); + } + ); + }); + }); + }); + }); +} From 31dd497099615f4529fa6b9e6f42a46443a3d5dc Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 10 Aug 2020 16:20:05 -0700 Subject: [PATCH 22/26] Adding idle socket timeout functional tests --- .../http/integration_tests/router.test.ts | 1 - .../server/plugin.ts | 51 ++++++++ .../test_suites/core/route.ts | 119 +++++++++++++++--- 3 files changed, 152 insertions(+), 19 deletions(-) diff --git a/src/core/server/http/integration_tests/router.test.ts b/src/core/server/http/integration_tests/router.test.ts index 40747aa3581a5..e19c348511f1a 100644 --- a/src/core/server/http/integration_tests/router.test.ts +++ b/src/core/server/http/integration_tests/router.test.ts @@ -400,7 +400,6 @@ describe('Options', () => { }, }, async (context, req, res) => { - await new Promise((resolve) => setTimeout(resolve, 2000)); return res.ok({}); } ); diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts index 10ffffcd6d41c..41c262872d21a 100644 --- a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts @@ -18,6 +18,7 @@ */ import { Plugin, CoreSetup } from 'kibana/server'; +import { schema } from '@kbn/config-schema'; export interface PluginARequestContext { ping: () => Promise; @@ -66,6 +67,56 @@ export class CorePluginRouteTimeoutsPlugin implements Plugin { return res.ok({}); } ); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 10 }, + }, + path: '/short_idle_socket_timeout', + validate: { + body: schema.maybe( + schema.object({ + responseDelay: schema.maybe(schema.number()), + }) + ), + }, + }, + async (context, req, res) => { + if (req.body.responseDelay) { + await new Promise((resolve) => setTimeout(resolve, req.body.responseDelay)); + } + return res.ok({}); + } + ); + + router.post( + { + options: { + body: { + accepts: ['application/json'], + }, + timeout: { idleSocket: 5000 }, + }, + path: '/longer_idle_socket_timeout', + validate: { + body: schema.maybe( + schema.object({ + responseDelay: schema.maybe(schema.number()), + }) + ), + }, + }, + async (context, req, res) => { + if (req.body.responseDelay) { + await new Promise((resolve) => setTimeout(resolve, req.body.responseDelay)); + } + return res.ok({}); + } + ); } public start() {} diff --git a/test/plugin_functional/test_suites/core/route.ts b/test/plugin_functional/test_suites/core/route.ts index 20daf84cdf0cc..becde49a69714 100644 --- a/test/plugin_functional/test_suites/core/route.ts +++ b/test/plugin_functional/test_suites/core/route.ts @@ -26,26 +26,27 @@ export default function ({ getService }: PluginFunctionalProviderContext) { describe('route', function () { describe('timeouts', function () { - describe('payload', function () { - const writeBodyCharAtATime = (request: Test, body: string, interval: number) => { - return new Promise((resolve, reject) => { - let i = 0; - const intervalId = setInterval(() => { - if (i < body.length) { - request.write(body[i++]); - } else { - clearInterval(intervalId); - request.end((err, res) => { - resolve(res); - }); - } - }, interval); - request.on('error', (err) => { + const writeBodyCharAtATime = (request: Test, body: string, interval: number) => { + return new Promise((resolve, reject) => { + let i = 0; + const intervalId = setInterval(() => { + if (i < body.length) { + request.write(body[i++]); + } else { clearInterval(intervalId); - reject(err); - }); + request.end((err, res) => { + resolve(res); + }); + } + }, interval); + request.on('error', (err) => { + clearInterval(intervalId); + reject(err); }); - }; + }); + }; + + describe('payload', function () { it(`should timeout if POST payload sending is too slow`, async () => { // start the request const request = supertest @@ -86,6 +87,88 @@ export default function ({ getService }: PluginFunctionalProviderContext) { ); }); }); + + describe('idle socket', function () { + it('should timeout if payload sending has too long of an idle period', async function () { + // start the request + const request = supertest + .post('/short_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":100}', 20); + + await result.then( + (res) => { + expect(res).to.be(undefined); + }, + (err) => { + expect(err.message).to.be('socket hang up'); + } + ); + }); + + it('should not timeout if payload sending does not have too long of an idle period', async function () { + // start the request + const request = supertest + .post('/longer_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":0}', 10); + + await result.then( + (res) => { + expect(res).to.have.property('statusCode', 200); + }, + (err) => { + expect(err).to.be(undefined); + } + ); + }); + + it('should timeout if servers response is too slow', async function () { + // start the request + const request = supertest + .post('/short_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":100}', 0); + + await result.then( + (res) => { + expect(res).to.be(undefined); + }, + (err) => { + expect(err.message).to.be('socket hang up'); + } + ); + }); + + it('should not timeout if servers response is fast enough', async function () { + // start the request + const request = supertest + .post('/longer_idle_socket_timeout') + .set('Content-Type', 'application/json') + .set('Transfer-Encoding', 'chunked') + .set('kbn-xsrf', 'true'); + + const result = writeBodyCharAtATime(request, '{"responseDelay":100}', 0); + + await result.then( + (res) => { + expect(res).to.have.property('statusCode', 200); + }, + (err) => { + expect(err).to.be(undefined); + } + ); + }); + }); }); }); } From 25b6db0e85cbaec8097b385ffd53dd76d4423458 Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 10 Aug 2020 16:38:18 -0700 Subject: [PATCH 23/26] Adding better comments, using ?? instead of || --- src/core/server/http/http_server.ts | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts index 1cc15f806a166..99ab0ef16c2f9 100644 --- a/src/core/server/http/http_server.ts +++ b/src/core/server/http/http_server.ts @@ -168,7 +168,10 @@ export class HttpServer { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), }; - // Works around https://github.com/hapijs/hapi/issues/4122 until v20 + // To work around https://github.com/hapijs/hapi/issues/4122 until v20, set the socket + // timeout on the route to a fake timeout only when the payload timeout is specified. + // Within the onPreAuth lifecycle of the route itself, we'll override the timeout with the + // real socket timeout. const fakeSocketTimeout = timeout?.payload ? timeout.payload + 1 : undefined; this.server.route({ @@ -181,10 +184,13 @@ export class HttpServer { ext: { onPreAuth: { method: (request, h) => { + // At this point, the socket timeout has only been set to work-around the HapiJS bug. + // We need to either set the real per-route timeout or use the default idle socket timeout if (timeout?.idleSocket) { - request.raw.req.socket.setTimeout(timeout?.idleSocket); + request.raw.req.socket.setTimeout(timeout.idleSocket); } else if (fakeSocketTimeout) { - request.raw.req.socket.setTimeout(this.config!.socketTimeout || 0); + // NodeJS uses a socket timeout of `0` to denote "no timeout" + request.raw.req.socket.setTimeout(this.config!.socketTimeout ?? 0); } return h.continue; From d4e7d2a501fdf2587de52aafa07e893dae234306 Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 10 Aug 2020 16:44:52 -0700 Subject: [PATCH 24/26] Fixing the plugin fixture TS --- .../plugins/core_plugin_route_timeouts/tsconfig.json | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json index 4a564ee1e5578..7647e8c8f1838 100644 --- a/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json @@ -5,10 +5,7 @@ "skipLibCheck": true }, "include": [ - "index.ts", - "public/**/*.ts", - "public/**/*.tsx", - "../../../../typings/**/*" + "server/**/*.ts" ], "exclude": [] } From 286238785e8dff1a22426ef150b29f2afa54185e Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 10 Aug 2020 18:45:19 -0700 Subject: [PATCH 25/26] Fixing some typescript errors --- .../plugins/core_plugin_route_timeouts/server/plugin.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts index 41c262872d21a..c5bdf6cce084c 100644 --- a/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/server/plugin.ts @@ -86,8 +86,8 @@ export class CorePluginRouteTimeoutsPlugin implements Plugin { }, }, async (context, req, res) => { - if (req.body.responseDelay) { - await new Promise((resolve) => setTimeout(resolve, req.body.responseDelay)); + if (req.body?.responseDelay) { + await new Promise((resolve) => setTimeout(resolve, req.body!.responseDelay)); } return res.ok({}); } @@ -111,8 +111,8 @@ export class CorePluginRouteTimeoutsPlugin implements Plugin { }, }, async (context, req, res) => { - if (req.body.responseDelay) { - await new Promise((resolve) => setTimeout(resolve, req.body.responseDelay)); + if (req.body?.responseDelay) { + await new Promise((resolve) => setTimeout(resolve, req.body!.responseDelay)); } return res.ok({}); } From 0b0c488ab43c297a3355da20bb80566ec7ac1d0f Mon Sep 17 00:00:00 2001 From: kobelb Date: Mon, 10 Aug 2020 18:50:46 -0700 Subject: [PATCH 26/26] Fixing plugin fixture tsconfig.json --- .../plugins/core_plugin_route_timeouts/tsconfig.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json index 7647e8c8f1838..d0751f31ecc5e 100644 --- a/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json +++ b/test/plugin_functional/plugins/core_plugin_route_timeouts/tsconfig.json @@ -5,7 +5,8 @@ "skipLibCheck": true }, "include": [ - "server/**/*.ts" + "server/**/*.ts", + "../../../../typings/**/*" ], "exclude": [] }