Added pressure handler to options #82
Conversation
|
The feature is good but CI is failing badly :( |
index.js
Outdated
| @@ -229,3 +245,5 @@ module.exports = fp(underPressure, { | |||
| fastify: '3.x', | |||
| name: 'under-pressure' | |||
| }) | |||
|
|
|||
| Object.assign(module.exports, { TYPE_EVENT_LOOP_DELAY, TYPE_EVENT_LOOP_UTILIZATION, TYPE_HEALTH_CHECK, TYPE_HEAP_USED_BYTES, TYPE_RSS_BYTES }) | |||
There was a problem hiding this comment.
Please use explicit module.exports.SOMETHING = statements, so that they can be used with native esm.
test.js
Outdated
|
|
||
| fastify.get('/', (req, rep) => rep.send('A')) | ||
|
|
||
| fastify.inject().get('/').end().then(res => t.equal(res.body, 'A')) |
There was a problem hiding this comment.
please use async/await instead
test.js
Outdated
| setTimeout(() => { | ||
| reject(new Error(errorMessage)) | ||
| }, 250) | ||
| }) |
There was a problem hiding this comment.
please use async/await and
const { promisify } = require('util')
const sleep = promisify(setTimeout)
index.js
Outdated
| const result = pressureHandler(req, reply, type, value) | ||
| if (result instanceof Promise) { | ||
| result.then(() => next()).catch(next) | ||
| } else { |
There was a problem hiding this comment.
I think you should handle the case where result is something else of a promise.
There was a problem hiding this comment.
But how? What am I supposed to do with a non-Promise? Calling reply.send() with the object/string/etc?
There was a problem hiding this comment.
Either that or error the response
Co-authored-by: Matteo Collina <[email protected]>
|
CI is falling on every environment, there is a typo. |
Guess why |
-used async/await
|
These test are quite tricky. Sometimes they fail and sometimes they pass even if I didn't change the code. Even local. |
|
Again 7/9. But this time other environments failed |
test.js
Outdated
| t.error(err) | ||
| fastify.server.unref() | ||
|
|
||
| if (monitorEventLoopDelay) { |
There was a problem hiding this comment.
I think if this is not available we should skip the test.
There was a problem hiding this comment.
done but it didnt fix the failing tests completely
Co-authored-by: Matteo Collina <[email protected]>
|
The tests fails for me locally as well on Mac, it's not a flake. |
|
I recommend to split test.js into two (or more) files under the test/ folder. I suspect there might be a few problems of concurrency/interaction between the tests. You might also want to rebase on top of master, as well as adding disabling parallelism of tests in tap. |
master merge
|
Finally all tests are green. |
|
Deal! Thanks for bearing with me. This module is really tricky. |
Heyho,
I used this plugin in one of my projects and I don't know why there is no way to just add a handler to the options to handle the 'pressure'. There was no way to determine what kind of pressure happend (eventLoopDelay, heapBytes, ...) and you could not handle the pressure and proceed the request the usual way.
So I added a way to add a pressure handler to the options.
Checklist
npm run testandnpm run benchmarkand the Code of conduct