From 697adc372afca1681653930f84ac9e74c47eda2a Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 18 Apr 2022 23:42:03 +0200 Subject: [PATCH 01/19] chore: expand gitignore --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 6704566..01c7ee1 100644 --- a/.gitignore +++ b/.gitignore @@ -102,3 +102,6 @@ dist # TernJS port file .tern-port + +# Not necessary for libs +package-lock.json \ No newline at end of file From 560b4c482ddbe7566275848f243239825714b2fa Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 18 Apr 2022 23:42:57 +0200 Subject: [PATCH 02/19] feat!: implement first version --- index.js | 134 +++++++++++++++++++++++++++++++++++++++++++++++++++ lib/index.js | 10 ++++ 2 files changed, 144 insertions(+) diff --git a/index.js b/index.js index ccacec3..eef5fbd 100644 --- a/index.js +++ b/index.js @@ -1 +1,135 @@ 'use strict' +const fp = require('fastify-plugin') + +const { Errors } = require('./lib/index') + +module.exports = fp( + function fastifyRacePlugin (fastify, globalOpts, next) { + const controllers = new Map() + let error + + globalOpts = Object.assign( + {}, + { handleOnError: true, onRequestClosed: null }, + globalOpts + ) + + if (typeof globalOpts !== 'object') { + error = new Errors.BAD_PARAMS('object', typeof globalOpts) + } else if (typeof globalOpts.handleOnError !== 'boolean') { + error = new Errors.BAD_PARAMS('boolean', typeof globalOpts.handleOnError) + } else if ( + globalOpts.onRequestClosed != null && + typeof globalOpts.onRequestClosed !== 'function' + ) { + error = new Errors.BAD_PARAMS( + 'function', + typeof globalOpts.onRequestClosed + ) + } + + fastify.decorateRequest('race', race) + + return next(error) + + function cleaner (cb) { + if (Array.isArray(this.onSend)) { + this.onSend.push(onSendCleaner) + } else if (this.onSend != null) { + this.onSend = [onSendCleaner, this.onSend] + } else { + this.onSend = onSendCleaner + } + + function onSendCleaner (request, _reply, payload, done) { + if (controllers.has(request.id)) { + const controllerSignal = controllers.get(request.id) + controllerSignal.removeEventListener('abort', cb, { + once: true + }) + controllers.delete(request.id) + } + + done(null, payload) + } + } + + function race (opts = globalOpts) { + const { raw, id: reqId } = this + const handleError = typeof opts === 'function' ? true : opts.handleOnError + const cb = typeof opts === 'function' ? opts : opts.onRequestClosed + + const controller = controllers.has(reqId) + ? controllers.get(reqId) // eslint-disable-next-line no-undef + : (controllers.set(reqId, new AbortController()), + controllers.get(reqId)) + const bindedCleaner = cleaner.bind(this) + + if (cb != null && typeof cb !== 'function') { + throw new Errors.BAD_PARAMS('function', typeof cb) + } + + if (cb != null) { + controller.signal.addEventListener('abort', cb, { + once: true + }) + + bindedCleaner(cb) + } + + if (cb == null) controller.signal.then = theneable.bind(this) + + if (raw.socket.destroyed) { + controller.abort(new Error('Socket already closed')) + } else { + raw.once('close', () => { + if (controller.signal.aborted === false) controller.abort() + + if (controllers.has(reqId)) { + const controllerSignal = controllers.get(reqId).signal + controllerSignal.removeEventListener('abort', cb, { + once: true + }) + controllers.delete(reqId) + } + }) + + if (handleError || cb != null) { + raw.once('error', err => { + if (controller.signal.aborted === false) controller.abort(err) + }) + } + } + + return controller.signal + + function theneable (resolve) { + try { + const theneableHandler = evt => { + resolve(evt) + + if (controllers.has(reqId)) { + const controllerSignal = controllers.get(reqId).signal + controllerSignal.removeEventListener('abort', cb, { + once: true + }) + controllers.delete(reqId) + } + } + + controller.signal.throwIfAborted() + controller.signal.addEventListener('abort', theneableHandler, { + once: true + }) + bindedCleaner(theneableHandler) + } catch (err) { + resolve(err) + } + } + } + }, + { + fastify: '>=3.24.1', + name: 'fastify-racing' + } +) diff --git a/lib/index.js b/lib/index.js index e69de29..1217516 100644 --- a/lib/index.js +++ b/lib/index.js @@ -0,0 +1,10 @@ +const Errors = require('fastify-error') + +module.exports = { + Errors: { + BAD_PARAMS: Errors( + 'FST_RACE_BAD_PARAM', + 'Invalid param, expected %s but received %s' + ) + } +} From 53c98f2eefed8e78455291046c777bec38b14531 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Mon, 18 Apr 2022 23:43:31 +0200 Subject: [PATCH 03/19] chore: update package metadata --- package.json | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 29ad2ab..e3a9062 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { - "name": "", + "name": "fastify-racing", "version": "0.0.0", - "description": "", + "description": "Cancel any running operation at the right time on your request handler", "main": "index.js", "types": "index.d.ts", "scripts": { @@ -16,17 +16,19 @@ "keywords": [], "repository": { "type": "git", - "url": "git+https://github.com/metcoder95/.git" + "url": "git+https://github.com/metcoder95/fastify-racing.git" }, - "readme": "https://github.com/metcoder95//blob/main/README.md", + "readme": "https://github.com/metcoder95/fastify-racing/blob/main/README.md", "bugs": { - "url": "https://github.com/metcoder95//issues" + "url": "https://github.com/metcoder95/fastify-racing/issues" }, "author": "metcoder95 ", "license": "MIT", "devDependencies": { - "@types/node": "^14.17.6", + "@types/node": "^14.18.13", + "fastify": "^3.28.0", "husky": "^7.0.2", + "nodemon": "^2.0.15", "snazzy": "^9.0.0", "standard": "^16.0.3", "tap": "^15.0.10", @@ -34,6 +36,8 @@ "typescript": "^4.4" }, "dependencies": { + "fastify-error": "^1.1.0", + "fastify-plugin": "^3.0.1" }, "tsd": { "directory": "test" From 5d0bc873a4c6b55a5d8c9ae0573b839d5bc7e89b Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 19 Apr 2022 22:57:44 +0200 Subject: [PATCH 04/19] test: set decoration testing --- index.js | 8 +++-- lib/index.js | 2 +- test/index.test.js | 73 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 4 deletions(-) diff --git a/index.js b/index.js index eef5fbd..126b52f 100644 --- a/index.js +++ b/index.js @@ -8,15 +8,17 @@ module.exports = fp( const controllers = new Map() let error + if (typeof globalOpts !== 'object') { + return next(new Errors.BAD_PARAMS('object', typeof globalOpts)) + } + globalOpts = Object.assign( {}, { handleOnError: true, onRequestClosed: null }, globalOpts ) - if (typeof globalOpts !== 'object') { - error = new Errors.BAD_PARAMS('object', typeof globalOpts) - } else if (typeof globalOpts.handleOnError !== 'boolean') { + if (typeof globalOpts.handleOnError !== 'boolean') { error = new Errors.BAD_PARAMS('boolean', typeof globalOpts.handleOnError) } else if ( globalOpts.onRequestClosed != null && diff --git a/lib/index.js b/lib/index.js index 1217516..e5357dd 100644 --- a/lib/index.js +++ b/lib/index.js @@ -3,7 +3,7 @@ const Errors = require('fastify-error') module.exports = { Errors: { BAD_PARAMS: Errors( - 'FST_RACE_BAD_PARAM', + 'FST_PLUGIN_RACE_BAD_PARAM', 'Invalid param, expected %s but received %s' ) } diff --git a/test/index.test.js b/test/index.test.js index ccacec3..b344c21 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -1 +1,74 @@ 'use strict' +const tap = require('tap') +const fastify = require('fastify') + +const plugin = require('../.') + +tap.plan(1) + +tap.test('fastify-racing#decoration', subtest => { + subtest.plan(4) + + subtest.test('Should decorate the request properly', async t => { + t.plan(3) + + const app = fastify() + app.register(plugin) + + app.get('/', (req, reply) => { + t.ok(req.race, 'should decorate request object') + t.equal(typeof req.race, 'function', 'should be a function') + + return 'hello' + }) + + const response = await app.inject({ + method: 'GET', + path: '/' + }) + + t.equal(response.body, 'hello') + }) + + subtest.test('Should throw if invalid Global opts', async t => { + t.plan(3) + + const app = fastify() + try { + await app.register(plugin, 'invalid').ready() + } catch (err) { + t.ok(err, 'should throw') + t.equal(err.code, 'FST_PLUGIN_RACE_BAD_PARAM') + t.equal(err.message, 'Invalid param, expected object but received string') + } + }) + + subtest.test('Should throw if invalid Global opts.handleOnError', async t => { + t.plan(3) + + const app = fastify() + try { + await app.register(plugin, { handleOnError: 'invalid' }).ready() + } catch (err) { + t.ok(err, 'should throw') + t.equal(err.code, 'FST_PLUGIN_RACE_BAD_PARAM') + t.equal(err.message, 'Invalid param, expected boolean but received string') + } + }) + + subtest.test('Should throw if invalid Global opts.onRequestClosed', async t => { + t.plan(3) + + const app = fastify() + try { + await app.register(plugin, { onRequestClosed: 1 }).ready() + } catch (err) { + t.ok(err, 'should throw') + t.equal(err.code, 'FST_PLUGIN_RACE_BAD_PARAM') + t.equal( + err.message, + 'Invalid param, expected function but received number' + ) + } + }) +}) From 7f44e91105f36bebc212e5d6a136399426238810 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 20 Apr 2022 00:00:20 +0200 Subject: [PATCH 05/19] refactor: validations --- index.js | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/index.js b/index.js index 126b52f..fa2761f 100644 --- a/index.js +++ b/index.js @@ -8,7 +8,7 @@ module.exports = fp( const controllers = new Map() let error - if (typeof globalOpts !== 'object') { + if (globalOpts != null && typeof globalOpts !== 'object') { return next(new Errors.BAD_PARAMS('object', typeof globalOpts)) } @@ -67,10 +67,6 @@ module.exports = fp( controllers.get(reqId)) const bindedCleaner = cleaner.bind(this) - if (cb != null && typeof cb !== 'function') { - throw new Errors.BAD_PARAMS('function', typeof cb) - } - if (cb != null) { controller.signal.addEventListener('abort', cb, { once: true From a641e6deec4c4229539248d6a43ab99242f5681a Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Wed, 20 Apr 2022 00:00:42 +0200 Subject: [PATCH 06/19] test: add testing for aborted request --- package.json | 3 +- test/index.test.js | 87 +++++++++++++++++++++++++++++++++++++++------- 2 files changed, 76 insertions(+), 14 deletions(-) diff --git a/package.json b/package.json index e3a9062..faac9a7 100644 --- a/package.json +++ b/package.json @@ -33,7 +33,8 @@ "standard": "^16.0.3", "tap": "^15.0.10", "tsd": "^0.17.0", - "typescript": "^4.4" + "typescript": "^4.4", + "undici": "^5.0.0" }, "dependencies": { "fastify-error": "^1.1.0", diff --git a/test/index.test.js b/test/index.test.js index b344c21..3b9f2e7 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -1,10 +1,13 @@ 'use strict' +const { setTimeout } = require('timers/promises') + const tap = require('tap') const fastify = require('fastify') +const { request } = require('undici') const plugin = require('../.') -tap.plan(1) +tap.plan(2) tap.test('fastify-racing#decoration', subtest => { subtest.plan(4) @@ -52,23 +55,81 @@ tap.test('fastify-racing#decoration', subtest => { } catch (err) { t.ok(err, 'should throw') t.equal(err.code, 'FST_PLUGIN_RACE_BAD_PARAM') - t.equal(err.message, 'Invalid param, expected boolean but received string') + t.equal( + err.message, + 'Invalid param, expected boolean but received string' + ) } }) - subtest.test('Should throw if invalid Global opts.onRequestClosed', async t => { + subtest.test( + 'Should throw if invalid Global opts.onRequestClosed', + async t => { + t.plan(3) + + const app = fastify() + try { + await app.register(plugin, { onRequestClosed: 1 }).ready() + } catch (err) { + t.ok(err, 'should throw') + t.equal(err.code, 'FST_PLUGIN_RACE_BAD_PARAM') + t.equal( + err.message, + 'Invalid param, expected function but received number' + ) + } + } + ) +}) + +tap.test('fastify-racing#promise', subtest => { + subtest.plan(1) + + subtest.test('Should handle a request aborted', t => { t.plan(3) const app = fastify() - try { - await app.register(plugin, { onRequestClosed: 1 }).ready() - } catch (err) { - t.ok(err, 'should throw') - t.equal(err.code, 'FST_PLUGIN_RACE_BAD_PARAM') - t.equal( - err.message, - 'Invalid param, expected function but received number' - ) - } + // eslint-disable-next-line no-undef + const abtCtlr = new AbortController() + app.register(plugin) + + t.teardown(() => app.close()) + + app.get('/', async (req, reply) => { + const signal = req.race() + const result = await Promise.race([signal, dummy(signal)]) + + t.equal(typeof result, 'object') + t.equal(result.type, 'abort') + + if (result.type === 'aborted') return '' + else return `${result}-world` + }) + + app + .ready() + .then(() => app.listen()) + .then(async () => { + request( + `http://localhost:${app.server.address().port}`, + { + method: 'GET', + path: '/', + signal: abtCtlr.signal + }, + (err, response, body) => { + t.ok(err) + } + ) + + // Allow a full event loop cycle + await setTimeout(5) + abtCtlr.abort() + }) }) + + async function dummy (signal) { + await setTimeout(3000, null, { signal }) + return 'hello' + } }) From 8af28f3b4734cccb3d10f8d7ef3ddee497d11261 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 10 May 2022 17:08:14 +0200 Subject: [PATCH 07/19] refactor: cleanup of not used controllers --- index.js | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/index.js b/index.js index fa2761f..9c69415 100644 --- a/index.js +++ b/index.js @@ -31,29 +31,26 @@ module.exports = fp( } fastify.decorateRequest('race', race) + fastify.addHook('onResponse', onResponseCleaner) return next(error) - function cleaner (cb) { - if (Array.isArray(this.onSend)) { - this.onSend.push(onSendCleaner) - } else if (this.onSend != null) { - this.onSend = [onSendCleaner, this.onSend] - } else { - this.onSend = onSendCleaner - } + function onResponseCleaner (request, _reply, done) { + if (controllers.has(request.id)) { + const { controller, cbs } = controllers.get(request.id) - function onSendCleaner (request, _reply, payload, done) { - if (controllers.has(request.id)) { - const controllerSignal = controllers.get(request.id) - controllerSignal.removeEventListener('abort', cb, { - once: true - }) - controllers.delete(request.id) + if (controller.signal.aborted === false) { + for (const cb of cbs) { + controller.signal.removeEventListener('abort', cb, { + once: true + }) + } } - done(null, payload) + controllers.delete(request.id) } + + done() } function race (opts = globalOpts) { From a73125e5088e2978f9084bba83f2ed304274c3af Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Tue, 10 May 2022 17:09:53 +0200 Subject: [PATCH 08/19] refactor: implement refactor with proper tests --- index.js | 112 +++++++++++++++----------- lib/index.js | 8 ++ test/index.test.js | 191 +++++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 260 insertions(+), 51 deletions(-) diff --git a/index.js b/index.js index 9c69415..9257f6c 100644 --- a/index.js +++ b/index.js @@ -58,67 +58,89 @@ module.exports = fp( const handleError = typeof opts === 'function' ? true : opts.handleOnError const cb = typeof opts === 'function' ? opts : opts.onRequestClosed - const controller = controllers.has(reqId) - ? controllers.get(reqId) // eslint-disable-next-line no-undef - : (controllers.set(reqId, new AbortController()), - controllers.get(reqId)) - const bindedCleaner = cleaner.bind(this) - - if (cb != null) { - controller.signal.addEventListener('abort', cb, { - once: true - }) - - bindedCleaner(cb) - } - - if (cb == null) controller.signal.then = theneable.bind(this) + if (controllers.has(reqId)) { + const { controller: ctrl, cbs } = controllers.get(reqId) - if (raw.socket.destroyed) { - controller.abort(new Error('Socket already closed')) - } else { - raw.once('close', () => { - if (controller.signal.aborted === false) controller.abort() - - if (controllers.has(reqId)) { - const controllerSignal = controllers.get(reqId).signal - controllerSignal.removeEventListener('abort', cb, { - once: true - }) - controllers.delete(reqId) - } - }) + if (cb != null) { + // TODO: handle when socket is destroyed already + ctrl.signal.addEventListener('abort', cb, { + once: true + }) - if (handleError || cb != null) { raw.once('error', err => { - if (controller.signal.aborted === false) controller.abort(err) + if (controllers.has(reqId)) { + const internalCtrl = controllers.get(reqId) + if (internalCtrl.signal.aborted === false) internalCtrl.abort(err) + } }) + + controllers.set(reqId, { controller: ctrl, cbs: cbs.concat(cb) }) } - } - return controller.signal + if (raw.socket.destroyed === true) { + process.nextTick(() => ctrl.abort(Errors.SOCKET_CLOSED(reqId))) + } - function theneable (resolve) { - try { - const theneableHandler = evt => { - resolve(evt) + return ctrl.signal + } else { + // eslint-disable-next-line no-undef + const controller = new AbortController() + + if (cb != null) { + controller.signal.addEventListener('abort', cb, { + once: true + }) + } + + if (cb == null) controller.signal.then = theneable.bind(this) + if (raw.socket.destroyed) { + process.nextTick(() => controller.abort(Errors.SOCKET_CLOSED(reqId))) + } else { + raw.once('close', () => { if (controllers.has(reqId)) { - const controllerSignal = controllers.get(reqId).signal - controllerSignal.removeEventListener('abort', cb, { - once: true - }) - controllers.delete(reqId) + const { controller: ctrl } = controllers.get(reqId) + if (ctrl.signal.aborted === false) controller.abort() } + }) + + if (handleError === true || cb != null) { + raw.once('error', err => { + if (controllers.has(reqId)) { + const { controller: ctrl } = controllers.get(reqId) + if (ctrl.signal.aborted === false) controller.abort(err) + } + }) } + } + + controllers.set(reqId, { controller, cbs: cb != null ? [cb] : [] }) + + return controller.signal + } + + function theneable (resolve, reject) { + const { controller, cbs } = controllers.get(reqId) - controller.signal.throwIfAborted() + if (controller.signal.aborted === true) { + return reject(Errors.ALREADY_ABORTED(reqId)) + } + + try { controller.signal.addEventListener('abort', theneableHandler, { once: true }) - bindedCleaner(theneableHandler) + + controllers.set(reqId, { + controller, + cbs: cbs.concat(theneableHandler) + }) } catch (err) { - resolve(err) + reject(err) + } + + function theneableHandler (evt) { + resolve(evt) } } } diff --git a/lib/index.js b/lib/index.js index e5357dd..71cbcb2 100644 --- a/lib/index.js +++ b/lib/index.js @@ -5,6 +5,14 @@ module.exports = { BAD_PARAMS: Errors( 'FST_PLUGIN_RACE_BAD_PARAM', 'Invalid param, expected %s but received %s' + ), + ALREADY_ABORTED: Errors( + 'FST_PLUGIN_RACE_ALREADY_ABORTED', + "Request with ID '%s' already aborted" + ), + SOCKET_CLOSED: Errors( + 'FST_PLUGIN_RACE_SOCKET_CLOSED', + "Socket for request with ID '%s' already closed" ) } } diff --git a/test/index.test.js b/test/index.test.js index 3b9f2e7..ce6c4c8 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -82,8 +82,10 @@ tap.test('fastify-racing#decoration', subtest => { ) }) -tap.test('fastify-racing#promise', subtest => { - subtest.plan(1) +// TODO: find what's hanging the tests +// TODO: remove "only" once done +tap.test('fastify-racing#promise', { only: true }, subtest => { + subtest.plan(4) subtest.test('Should handle a request aborted', t => { t.plan(3) @@ -95,7 +97,7 @@ tap.test('fastify-racing#promise', subtest => { t.teardown(() => app.close()) - app.get('/', async (req, reply) => { + app.get('/', async (req, _reply) => { const signal = req.race() const result = await Promise.race([signal, dummy(signal)]) @@ -117,7 +119,7 @@ tap.test('fastify-racing#promise', subtest => { path: '/', signal: abtCtlr.signal }, - (err, response, body) => { + err => { t.ok(err) } ) @@ -128,8 +130,185 @@ tap.test('fastify-racing#promise', subtest => { }) }) - async function dummy (signal) { - await setTimeout(3000, null, { signal }) + subtest.test( + 'Should be able to handle more than one race check within a request', + t => { + const app = fastify() + // eslint-disable-next-line no-undef + const abtCtlr = new AbortController() + let starter + + t.plan(10) + + app.register(plugin) + + app.get( + '/', + { + preHandler: [ + async (req, _reply) => { + starter = req.race() + const result = await Promise.race([starter, dummy(starter, 10)]) + t.equal(result, 'hello') + }, + async (req, _reply) => { + const second = req.race() + const result = await Promise.race([second, dummy(second, 10)]) + + t.equal(result, 'hello') + t.equal( + starter, + second, + 'Should use the same AbortController instance' + ) + }, + async (req, _reply) => { + const third = req.race() + const result = await Promise.race([third, dummy(third, 10)]) + t.equal(result, 'hello') + t.equal( + starter, + third, + 'Should use the same AbortController instance' + ) + } + ] + }, + async (req, _reply) => { + const final = req.race() + + const result = await Promise.race([final, dummy(final, 2000)]) + + t.ok(final.aborted) + t.equal(final, starter, 'Should reuse the initial controller') + + t.equal(typeof result, 'object') + t.equal(result.type, 'abort') + + return '' + } + ) + + t.teardown(() => app.close()) + + app + .ready() + .then(() => app.listen()) + .then(async () => { + request( + `http://localhost:${app.server.address().port}`, + { + method: 'GET', + path: '/', + signal: abtCtlr.signal + }, + err => { + t.ok(err) + } + ) + + // Allow a full event loop cycle + await setTimeout(500) + abtCtlr.abort() + }) + } + ) + + subtest.test( + 'Should reuse AbortController for the single request', + async t => { + let first + const app = fastify() + + t.plan(5) + + app.register(plugin) + + app.get( + '/', + { + preHandler: (req, _reply, done) => { + first = req.race() + + t.ok(first) + done() + } + }, + (req, _reply) => { + const second = req.race() + + t.notOk(second.aborted) + t.equal(second, first, 'Should reuse the initial controller') + + return 'Hello World' + } + ) + + t.teardown(() => app.close()) + + await app.listen() + + const response = await request( + `http://localhost:${app.server.address().port}`, + { + method: 'GET', + path: '/' + } + ) + + t.equal(response.statusCode, 200) + t.equal(await response.body.text(), 'Hello World') + t.end() + } + ) + + // TODO: Find how to close the socket after request finished + subtest.test('Should throw on already closed request', { skipn: true }, async t => { + let first + const app = fastify() + + t.plan(5) + + app.register(plugin) + + app.get( + '/', + { + onResponse: (req, _reply, done) => { + try { + console.log('Triggering failure') + first = req.race() + } catch (err) { t.ok(err) } + + t.notOk(first) + done() + } + }, + (req, _reply) => { + process._rawDebug(req.headers) + return 'Hello World' + } + ) + + t.teardown(() => app.close()) + + await app.listen() + + const response = await request( + `http://localhost:${app.server.address().port}`, + { + method: 'GET', + path: '/' + } + ) + + t.equal(response.statusCode, 200) + t.equal(await response.body.text(), 'Hello World') + t.end() + }) + + async function dummy (signal, ms = 3000) { + await setTimeout(ms, null, { signal, ref: false }) return 'hello' } }) From a049c5497b1db5dcc2f989328510c82f278afe01 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 12:34:04 +0200 Subject: [PATCH 09/19] fix: handle socket closed on Promise returns --- index.js | 4 +++ test/index.test.js | 77 ++++++++++++++++++++++++++-------------------- 2 files changed, 47 insertions(+), 34 deletions(-) diff --git a/index.js b/index.js index 9257f6c..6f9b34c 100644 --- a/index.js +++ b/index.js @@ -122,6 +122,10 @@ module.exports = fp( function theneable (resolve, reject) { const { controller, cbs } = controllers.get(reqId) + if (raw.socket.destroyed === true) { + return reject(Errors.SOCKET_CLOSED(reqId)) + } + if (controller.signal.aborted === true) { return reject(Errors.ALREADY_ABORTED(reqId)) } diff --git a/test/index.test.js b/test/index.test.js index ce6c4c8..071beae 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -6,6 +6,7 @@ const fastify = require('fastify') const { request } = require('undici') const plugin = require('../.') +const { Errors } = require('../lib') tap.plan(2) @@ -263,49 +264,57 @@ tap.test('fastify-racing#promise', { only: true }, subtest => { ) // TODO: Find how to close the socket after request finished - subtest.test('Should throw on already closed request', { skipn: true }, async t => { - let first - const app = fastify() + subtest.test( + 'Should throw on already closed request', + { skip: false }, + async t => { + let first + const app = fastify() - t.plan(5) + t.plan(7) - app.register(plugin) + app.register(plugin) - app.get( - '/', - { - onResponse: (req, _reply, done) => { - try { - console.log('Triggering failure') - first = req.race() - } catch (err) { t.ok(err) } + app.get( + '/', + { + onResponse: async (req, _reply, done) => { + req.raw.destroy() - t.notOk(first) - done() + try { + first = await req.race() + } catch (err) { + t.ok(err) + t.ok(err instanceof Errors.SOCKET_CLOSED) + t.equal(err.code, 'FST_PLUGIN_RACE_SOCKET_CLOSED') + t.equal(err.statusCode, 500) + } + + t.notOk(first) + done() + } + }, + (req, _reply) => { + return 'Hello World' } - }, - (req, _reply) => { - process._rawDebug(req.headers) - return 'Hello World' - } - ) + ) - t.teardown(() => app.close()) + t.teardown(() => app.close()) - await app.listen() + await app.listen() - const response = await request( - `http://localhost:${app.server.address().port}`, - { - method: 'GET', - path: '/' - } - ) + const response = await request( + `http://localhost:${app.server.address().port}`, + { + method: 'GET', + path: '/' + } + ) - t.equal(response.statusCode, 200) - t.equal(await response.body.text(), 'Hello World') - t.end() - }) + t.equal(response.statusCode, 200) + t.equal(await response.body.text(), 'Hello World') + } + ) async function dummy (signal, ms = 3000) { await setTimeout(ms, null, { signal, ref: false }) From 9a80f947e821adb31fa5857d1a687ba3d43419f3 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 12:35:18 +0200 Subject: [PATCH 10/19] refactor: leftover --- test/index.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/index.test.js b/test/index.test.js index 071beae..becc644 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -266,7 +266,6 @@ tap.test('fastify-racing#promise', { only: true }, subtest => { // TODO: Find how to close the socket after request finished subtest.test( 'Should throw on already closed request', - { skip: false }, async t => { let first const app = fastify() From 74d821bd9d98f943f24aedcbc432c14deea40725 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 12:40:04 +0200 Subject: [PATCH 11/19] fix: support tests for nodev14 --- test/index.test.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/index.test.js b/test/index.test.js index becc644..e50df5e 100644 --- a/test/index.test.js +++ b/test/index.test.js @@ -1,5 +1,5 @@ 'use strict' -const { setTimeout } = require('timers/promises') +const { promisify } = require('util') const tap = require('tap') const fastify = require('fastify') @@ -8,6 +8,8 @@ const { request } = require('undici') const plugin = require('../.') const { Errors } = require('../lib') +const sleep = promisify(setTimeout) + tap.plan(2) tap.test('fastify-racing#decoration', subtest => { @@ -126,7 +128,7 @@ tap.test('fastify-racing#promise', { only: true }, subtest => { ) // Allow a full event loop cycle - await setTimeout(5) + await sleep(5) abtCtlr.abort() }) }) @@ -209,7 +211,7 @@ tap.test('fastify-racing#promise', { only: true }, subtest => { ) // Allow a full event loop cycle - await setTimeout(500) + await sleep(500) abtCtlr.abort() }) } @@ -316,7 +318,7 @@ tap.test('fastify-racing#promise', { only: true }, subtest => { ) async function dummy (signal, ms = 3000) { - await setTimeout(ms, null, { signal, ref: false }) + await sleep(ms, null, { signal, ref: false }) return 'hello' } }) From 78615398b6d9840a26f98e7df977457f6417f1e7 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 12:45:59 +0200 Subject: [PATCH 12/19] regressive: drop support for node14 --- .github/workflows/ci.yml | 2 +- package.json | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e603fc..50fedf7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,7 +6,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - node: [14.x, 16.x, 17.x] + node: [16.x, 17.x] name: Node ${{ matrix.node }} steps: - uses: actions/checkout@v1 diff --git a/package.json b/package.json index faac9a7..689d174 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,9 @@ "lint:ci": "standard", "typescript": "tsd" }, + "engines": { + "node": ">=16.0.0" + }, "keywords": [], "repository": { "type": "git", From 74a04cfcf448d29f729ef21cf24d4ff3b3da0c84 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 13:22:11 +0200 Subject: [PATCH 13/19] feat: add ts types --- test/index.test-d.ts | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/test/index.test-d.ts b/test/index.test-d.ts index e69de29..0f0b773 100644 --- a/test/index.test-d.ts +++ b/test/index.test-d.ts @@ -0,0 +1,26 @@ +/// +import { FastifyPluginCallback } from 'fastify'; +import { FastifyError } from 'fastify-error'; + + +interface AbortEvent { + type: 'abort' | string; + reason?: FastifyError | Error +} + +interface FastifyRacing { + handleError?: boolean; + onRequestClosed?: (evt: AbortEvent) => void; +} + +declare module 'fastify' { + interface FastifyInstance { + race(cb: FastifyRacing['onRequestClosed']): void + race(opts: FastifyRacing): Promise + race(): Promise + } +} + +declare const FastifyRacing: FastifyPluginCallback; + +export default FastifyRacing; \ No newline at end of file From 255c283307b6afb5ae8b66adabbd379ca482cdc4 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 13:24:01 +0200 Subject: [PATCH 14/19] refactor: apply refactoring to Abort event result --- index.js | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/index.js b/index.js index 6f9b34c..fa1b257 100644 --- a/index.js +++ b/index.js @@ -31,11 +31,11 @@ module.exports = fp( } fastify.decorateRequest('race', race) - fastify.addHook('onResponse', onResponseCleaner) + fastify.addHook('onResponse', fastifyRacingCleaner) return next(error) - function onResponseCleaner (request, _reply, done) { + function fastifyRacingCleaner (request, _reply, done) { if (controllers.has(request.id)) { const { controller, cbs } = controllers.get(request.id) @@ -61,26 +61,22 @@ module.exports = fp( if (controllers.has(reqId)) { const { controller: ctrl, cbs } = controllers.get(reqId) + if (ctrl.signal.aborted === true) { + throw new Errors.ALREADY_ABORTED(reqId) + } + + if (raw.socket.destroyed === true) { + throw new Errors.SOCKET_CLOSED(reqId) + } + if (cb != null) { - // TODO: handle when socket is destroyed already ctrl.signal.addEventListener('abort', cb, { once: true }) - raw.once('error', err => { - if (controllers.has(reqId)) { - const internalCtrl = controllers.get(reqId) - if (internalCtrl.signal.aborted === false) internalCtrl.abort(err) - } - }) - controllers.set(reqId, { controller: ctrl, cbs: cbs.concat(cb) }) } - if (raw.socket.destroyed === true) { - process.nextTick(() => ctrl.abort(Errors.SOCKET_CLOSED(reqId))) - } - return ctrl.signal } else { // eslint-disable-next-line no-undef @@ -95,7 +91,7 @@ module.exports = fp( if (cb == null) controller.signal.then = theneable.bind(this) if (raw.socket.destroyed) { - process.nextTick(() => controller.abort(Errors.SOCKET_CLOSED(reqId))) + throw new Errors.ALREADY_ABORTED(reqId) } else { raw.once('close', () => { if (controllers.has(reqId)) { @@ -144,7 +140,12 @@ module.exports = fp( } function theneableHandler (evt) { - resolve(evt) + const event = { + type: evt.type, + reason: controller.signal?.reason + } + + resolve(event) } } } From 7619625d9a9c63a5deb8c449c4eb1a91f81555c0 Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 13:27:12 +0200 Subject: [PATCH 15/19] fix: error thrown --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index fa1b257..8e54d89 100644 --- a/index.js +++ b/index.js @@ -91,7 +91,7 @@ module.exports = fp( if (cb == null) controller.signal.then = theneable.bind(this) if (raw.socket.destroyed) { - throw new Errors.ALREADY_ABORTED(reqId) + throw new Errors.SOCKET_CLOSED(reqId) } else { raw.once('close', () => { if (controllers.has(reqId)) { From bd8081cdecabcfe6c7eb5c35a3935d8c8ba6931a Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 13:35:38 +0200 Subject: [PATCH 16/19] chore: replace fastify-error --- lib/index.js | 2 +- package.json | 2 +- test/index.test-d.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/index.js b/lib/index.js index 71cbcb2..318c41d 100644 --- a/lib/index.js +++ b/lib/index.js @@ -1,4 +1,4 @@ -const Errors = require('fastify-error') +const Errors = require('@fastify/error') module.exports = { Errors: { diff --git a/package.json b/package.json index 689d174..fc9ec7e 100644 --- a/package.json +++ b/package.json @@ -40,7 +40,7 @@ "undici": "^5.0.0" }, "dependencies": { - "fastify-error": "^1.1.0", + "@fastify/error": "^2.0.0", "fastify-plugin": "^3.0.1" }, "tsd": { diff --git a/test/index.test-d.ts b/test/index.test-d.ts index 0f0b773..69e5d9d 100644 --- a/test/index.test-d.ts +++ b/test/index.test-d.ts @@ -1,6 +1,6 @@ /// import { FastifyPluginCallback } from 'fastify'; -import { FastifyError } from 'fastify-error'; +import { FastifyError } from '@fastify/error'; interface AbortEvent { From 9970f57980cb18d727697bf78d226918c8f7ea6d Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Thu, 12 May 2022 13:38:11 +0200 Subject: [PATCH 17/19] chore: add Node 18 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 50fedf7..2fe9d0b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,7 +6,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - node: [16.x, 17.x] + node: [16.x, 17.x, 18.x] name: Node ${{ matrix.node }} steps: - uses: actions/checkout@v1 From c9062aa35aa8452fcfa9b215e46e0fa49d21d2bd Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 13 May 2022 16:09:12 +0200 Subject: [PATCH 18/19] refactor: migrate to weak maps --- index.js | 54 ++++++++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/index.js b/index.js index 8e54d89..2514d51 100644 --- a/index.js +++ b/index.js @@ -5,7 +5,7 @@ const { Errors } = require('./lib/index') module.exports = fp( function fastifyRacePlugin (fastify, globalOpts, next) { - const controllers = new Map() + const controllers = new WeakMap() let error if (globalOpts != null && typeof globalOpts !== 'object') { @@ -36,8 +36,8 @@ module.exports = fp( return next(error) function fastifyRacingCleaner (request, _reply, done) { - if (controllers.has(request.id)) { - const { controller, cbs } = controllers.get(request.id) + if (controllers.has(request)) { + const { controller, cbs } = controllers.get(request) if (controller.signal.aborted === false) { for (const cb of cbs) { @@ -47,7 +47,7 @@ module.exports = fp( } } - controllers.delete(request.id) + controllers.delete(request) } done() @@ -58,8 +58,8 @@ module.exports = fp( const handleError = typeof opts === 'function' ? true : opts.handleOnError const cb = typeof opts === 'function' ? opts : opts.onRequestClosed - if (controllers.has(reqId)) { - const { controller: ctrl, cbs } = controllers.get(reqId) + if (controllers.has(this)) { + const { controller: ctrl, cbs } = controllers.get(this) if (ctrl.signal.aborted === true) { throw new Errors.ALREADY_ABORTED(reqId) @@ -74,7 +74,7 @@ module.exports = fp( once: true }) - controllers.set(reqId, { controller: ctrl, cbs: cbs.concat(cb) }) + controllers.set(this, { controller: ctrl, cbs: cbs.concat(cb) }) } return ctrl.signal @@ -93,37 +93,43 @@ module.exports = fp( if (raw.socket.destroyed) { throw new Errors.SOCKET_CLOSED(reqId) } else { - raw.once('close', () => { - if (controllers.has(reqId)) { - const { controller: ctrl } = controllers.get(reqId) - if (ctrl.signal.aborted === false) controller.abort() - } - }) + raw.once( + 'close', + function () { + if (controllers.has(this)) { + const { controller: ctrl } = controllers.get(this) + if (ctrl.signal.aborted === false) controller.abort() + } + }.bind(this) + ) if (handleError === true || cb != null) { - raw.once('error', err => { - if (controllers.has(reqId)) { - const { controller: ctrl } = controllers.get(reqId) - if (ctrl.signal.aborted === false) controller.abort(err) - } - }) + raw.once( + 'error', + function (err) { + if (controllers.has(this)) { + const { controller: ctrl } = controllers.get(this) + if (ctrl.signal.aborted === false) controller.abort(err) + } + }.bind(this) + ) } } - controllers.set(reqId, { controller, cbs: cb != null ? [cb] : [] }) + controllers.set(this, { controller, cbs: cb != null ? [cb] : [] }) return controller.signal } function theneable (resolve, reject) { - const { controller, cbs } = controllers.get(reqId) + const { controller, cbs } = controllers.get(this) if (raw.socket.destroyed === true) { - return reject(Errors.SOCKET_CLOSED(reqId)) + return reject(Errors.SOCKET_CLOSED(this.id)) } if (controller.signal.aborted === true) { - return reject(Errors.ALREADY_ABORTED(reqId)) + return reject(Errors.ALREADY_ABORTED(this.id)) } try { @@ -131,7 +137,7 @@ module.exports = fp( once: true }) - controllers.set(reqId, { + controllers.set(this, { controller, cbs: cbs.concat(theneableHandler) }) From 2428729cc7dc2eb075ea3567ebef309426db2cad Mon Sep 17 00:00:00 2001 From: Carlos Fuentes Date: Fri, 13 May 2022 16:57:38 +0200 Subject: [PATCH 19/19] ci: remov node 17 --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fe9d0b..48b3030 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,7 +6,7 @@ jobs: runs-on: ubuntu-latest strategy: matrix: - node: [16.x, 17.x, 18.x] + node: [16.x, 18.x] name: Node ${{ matrix.node }} steps: - uses: actions/checkout@v1