-
Notifications
You must be signed in to change notification settings - Fork 75
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add Koa Server Request Timeout #2504
Conversation
Signed-off-by: Nana Essilfie-Conduah <[email protected]>
packages/server/src/index.ts
Outdated
const server = await app.listen({ port: process.env.SERVER_PORT || 7546 }); | ||
|
||
// set request timeout to ensure sockets are closed after specified time | ||
const requestTimeoutMs = parseInt(process.env.SERVER_REQUEST_TIMEOUT_MS!) || 30000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set timeout appropriately.
Maybe 60 secs or more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
packages/server/src/index.ts
Outdated
|
||
// set request timeout to ensure sockets are closed after specified time | ||
const requestTimeoutMs = parseInt(process.env.SERVER_REQUEST_TIMEOUT_MS!) || 30000; | ||
server.setTimeout(requestTimeoutMs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a test to confirm relay sends timeout in this case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
c775567
to
b869431
Compare
@@ -2021,4 +2081,23 @@ describe('@api-batch-3 RPC Server Acceptance Tests', function () { | |||
} | |||
}); | |||
}); | |||
|
|||
describe.only('Koa Server Timeout', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove .only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, done.
@@ -2021,4 +2081,23 @@ describe('@api-batch-3 RPC Server Acceptance Tests', function () { | |||
} | |||
}); | |||
}); | |||
|
|||
describe.only('Koa Server Timeout', () => { | |||
it.only('should timeout a request after the specified time', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove .only
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -22,4 +22,5 @@ BATCH_REQUESTS_ENABLED=true | |||
TEST_GAS_PRICE_DEVIATION=0.2 | |||
WS_NEW_HEADS_ENABLED=false | |||
INITIAL_BALANCE='5000000000' | |||
LIMIT_DURATION=90000 | |||
LIMIT_DURATION=90000, | |||
SERVER_REQUEST_TIMEOUT_MS=3000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
chai.use(chaiExclude); | ||
|
||
const sendJsonRpcRequestWithDelay = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest moving this method to helpers/utils.js or somewhere else to keep the batch 3 file clean. Also remove unsed import statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -15,3 +15,4 @@ SUBSCRIPTIONS_ENABLED=true | |||
FILTER_API_ENABLED=true | |||
DEBUG_API_ENABLED=true | |||
TEST_GAS_PRICE_DEVIATION=0.2 | |||
SERVER_REQUEST_TIMEOUT_MS=60000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
empty line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
runnng the newly added test on local keep getting this error 1) RPC Server Acceptance Tests
Acceptance tests
@api-batch-3 RPC Server Acceptance Tests
Koa Server Timeout
should timeout a request after the specified time:
AssertionError: expected undefined to equal 'ECONNRESET'
at Suite.<anonymous> (packages/server/tests/acceptance/rpc_batch3.spec.ts:2038:29)
at Generator.next (<anonymous>)
at fulfilled (packages/server/tests/acceptance/rpc_batch3.spec.ts:47:58)
at processTicksAndRejections (node:internal/process/task_queues:95:5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we need to restore
import chaiExclude from 'chai-exclude';
packages/server/src/index.ts
Outdated
|
||
// set request timeout to ensure sockets are closed after specified time of inactivity | ||
const requestTimeoutMs = parseInt(process.env.SERVER_REQUEST_TIMEOUT_MS || '60000'); | ||
server.setTimeout(requestTimeoutMs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The setTimeout method allows to send a callback function that will be executed in case a timeout is executed:
I think we should use it to capture metrics of timeouts.
however, we don't have the registry yet in here...
so I was thinking if we can change the approach and do something like this??
I haven't tested this yet, but basically uses:
https://www.npmjs.com/package/koa-timeout
and move it to file Server.ts
and do it something like this:
// Custom middleware to increment the metric on timeout
app.use(async (ctx, next) => {
try {
await next();
} catch (err) {
if (err.message === 'Request timeout') {
// Increment the timeout metric here
// For example, using a metric library like Prometheus:
// prometheusClient.counter('request_timeouts_total').inc();
console.log('Request timeout reached');
}
throw err;
}
});
// Apply the timeout middleware
app.use(timeout(5000)); // Set the timeout value in milliseconds
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2504 +/- ##
==========================================
- Coverage 77.26% 76.99% -0.27%
==========================================
Files 40 40
Lines 3229 3226 -3
Branches 663 667 +4
==========================================
- Hits 2495 2484 -11
- Misses 525 526 +1
- Partials 209 216 +7 ☔ View full report in Codecov by Sentry. |
Signed-off-by: ebadiere <[email protected]> fix: Added doc updated. Signed-off-by: ebadiere <[email protected]> fix: Bumped timeout to 60 seconds. Signed-off-by: ebadiere <[email protected]> fix: removed only. Signed-off-by: ebadiere <[email protected]> fix: Added empty line. Signed-off-by: ebadiere <[email protected]> Update packages/server/src/index.ts Co-authored-by: Logan Nguyen <[email protected]> Signed-off-by: Eric Badiere <[email protected]> fix: Applied feedback Signed-off-by: ebadiere <[email protected]> fix: Added back the chai. Signed-off-by: ebadiere <[email protected]> fix: Restored the chai exclude Signed-off-by: ebadiere <[email protected]> fix: Removed the async in the sendJsonRpcRequestWithDelay Signed-off-by: ebadiere <[email protected]> fix: Reduced the timeout to allow tests to finish. Signed-off-by: ebadiere <[email protected]> fix: Moved timeout test to rateLimiter. Signed-off-by: ebadiere <[email protected]> fix: Removed comma after LIMIT_DURATION Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout delay to ensure it works in CI. Signed-off-by: ebadiere <[email protected]> fix: Testing in CI. Removed the .only Signed-off-by: ebadiere <[email protected]> fix: Adjusted the test delay. Signed-off-by: ebadiere <[email protected]> fix: Added error details. Signed-off-by: ebadiere <[email protected]> fix: Added more debug info. Signed-off-by: ebadiere <[email protected]> fix: Added delay for linux. Debugging in CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Setting test timeout Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Nested promise. Signed-off-by: ebadiere <[email protected]> fix: Including delay in testfile. Debugging CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CIfix: Debugging in CI. Updated package-log. Signed-off-by: ebadiere <[email protected]> fix: Extracted setting logic to util function to be used in server startup and test framework Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout for tests on local node. Signed-off-by: ebadiere <[email protected]>
f45244c
to
432089b
Compare
Signed-off-by: ebadiere <[email protected]>
Signed-off-by: ebadiere <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test passed 🎊 🥂!!!
Some questions and suggestions.
@@ -9307,6 +9307,7 @@ | |||
"resolved": "https://registry.npmjs.org/bufferutil/-/bufferutil-4.0.5.tgz", | |||
"integrity": "sha512-HTm14iMQKK2FjFLRTM5lAVcyaUzOnqbPtesFIvREgXpJHdQm8bWS+GkQgIkfaBYRHuCnea7w8UVNfwiAQhlr9A==", | |||
"dev": true, | |||
"hasInstallScript": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering how come we have these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package-lock.json was mentioned as a suspect when we met to discuss the failing test in CI, so I included it. The hasInstallScript
indicates that a package has an install script to run when installing.
@@ -158,4 +158,29 @@ describe('@ratelimiter Rate Limiters Acceptance Tests', function () { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('Koa Server Timeout', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we can cancel the server-config
CI and have this test here, or we keep the server-config
CI and remove this test in ratelimiter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. That got missed in the back and forth. Removing.
import { expect } from 'chai'; | ||
import { Utils } from '../helpers/utils'; | ||
|
||
describe('@server-config Rate Limiters Acceptance Tests', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either we can cancel the server-config
CI and have this test in ratelimiter, or we keep the server-config
CI and remove this test here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, see comment above and updated tests title.
Signed-off-by: ebadiere <[email protected]>
bb8125c
to
7fb85ec
Compare
Signed-off-by: ebadiere <[email protected]>
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* Add Koa Server Request Timeout Signed-off-by: Nana Essilfie-Conduah <[email protected]> * fix: Added test and documentation to the server request timeout setting. Signed-off-by: ebadiere <[email protected]> fix: Added doc updated. Signed-off-by: ebadiere <[email protected]> fix: Bumped timeout to 60 seconds. Signed-off-by: ebadiere <[email protected]> fix: removed only. Signed-off-by: ebadiere <[email protected]> fix: Added empty line. Signed-off-by: ebadiere <[email protected]> Update packages/server/src/index.ts Co-authored-by: Logan Nguyen <[email protected]> Signed-off-by: Eric Badiere <[email protected]> fix: Applied feedback Signed-off-by: ebadiere <[email protected]> fix: Added back the chai. Signed-off-by: ebadiere <[email protected]> fix: Restored the chai exclude Signed-off-by: ebadiere <[email protected]> fix: Removed the async in the sendJsonRpcRequestWithDelay Signed-off-by: ebadiere <[email protected]> fix: Reduced the timeout to allow tests to finish. Signed-off-by: ebadiere <[email protected]> fix: Moved timeout test to rateLimiter. Signed-off-by: ebadiere <[email protected]> fix: Removed comma after LIMIT_DURATION Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout delay to ensure it works in CI. Signed-off-by: ebadiere <[email protected]> fix: Testing in CI. Removed the .only Signed-off-by: ebadiere <[email protected]> fix: Adjusted the test delay. Signed-off-by: ebadiere <[email protected]> fix: Added error details. Signed-off-by: ebadiere <[email protected]> fix: Added more debug info. Signed-off-by: ebadiere <[email protected]> fix: Added delay for linux. Debugging in CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Setting test timeout Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Nested promise. Signed-off-by: ebadiere <[email protected]> fix: Including delay in testfile. Debugging CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CIfix: Debugging in CI. Updated package-log. Signed-off-by: ebadiere <[email protected]> fix: Extracted setting logic to util function to be used in server startup and test framework Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout for tests on local node. Signed-off-by: ebadiere <[email protected]> * fix: Moved into its own CI job. Signed-off-by: ebadiere <[email protected]> * fix: Cleanup Signed-off-by: ebadiere <[email protected]> * fix: Clean up. Signed-off-by: ebadiere <[email protected]> --------- Signed-off-by: Nana Essilfie-Conduah <[email protected]> Signed-off-by: ebadiere <[email protected]> Co-authored-by: ebadiere <[email protected]>
* Add Koa Server Request Timeout Signed-off-by: Nana Essilfie-Conduah <[email protected]> * fix: Added test and documentation to the server request timeout setting. Signed-off-by: ebadiere <[email protected]> fix: Added doc updated. Signed-off-by: ebadiere <[email protected]> fix: Bumped timeout to 60 seconds. Signed-off-by: ebadiere <[email protected]> fix: removed only. Signed-off-by: ebadiere <[email protected]> fix: Added empty line. Signed-off-by: ebadiere <[email protected]> Update packages/server/src/index.ts Co-authored-by: Logan Nguyen <[email protected]> Signed-off-by: Eric Badiere <[email protected]> fix: Applied feedback Signed-off-by: ebadiere <[email protected]> fix: Added back the chai. Signed-off-by: ebadiere <[email protected]> fix: Restored the chai exclude Signed-off-by: ebadiere <[email protected]> fix: Removed the async in the sendJsonRpcRequestWithDelay Signed-off-by: ebadiere <[email protected]> fix: Reduced the timeout to allow tests to finish. Signed-off-by: ebadiere <[email protected]> fix: Moved timeout test to rateLimiter. Signed-off-by: ebadiere <[email protected]> fix: Removed comma after LIMIT_DURATION Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout delay to ensure it works in CI. Signed-off-by: ebadiere <[email protected]> fix: Testing in CI. Removed the .only Signed-off-by: ebadiere <[email protected]> fix: Adjusted the test delay. Signed-off-by: ebadiere <[email protected]> fix: Added error details. Signed-off-by: ebadiere <[email protected]> fix: Added more debug info. Signed-off-by: ebadiere <[email protected]> fix: Added delay for linux. Debugging in CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Setting test timeout Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Nested promise. Signed-off-by: ebadiere <[email protected]> fix: Including delay in testfile. Debugging CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CIfix: Debugging in CI. Updated package-log. Signed-off-by: ebadiere <[email protected]> fix: Extracted setting logic to util function to be used in server startup and test framework Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout for tests on local node. Signed-off-by: ebadiere <[email protected]> * fix: Moved into its own CI job. Signed-off-by: ebadiere <[email protected]> * fix: Cleanup Signed-off-by: ebadiere <[email protected]> * fix: Clean up. Signed-off-by: ebadiere <[email protected]> --------- Signed-off-by: Nana Essilfie-Conduah <[email protected]> Signed-off-by: ebadiere <[email protected]> Co-authored-by: ebadiere <[email protected]> Signed-off-by: Logan Nguyen <[email protected]>
* Add Koa Server Request Timeout Signed-off-by: Nana Essilfie-Conduah <[email protected]> * fix: Added test and documentation to the server request timeout setting. Signed-off-by: ebadiere <[email protected]> fix: Added doc updated. Signed-off-by: ebadiere <[email protected]> fix: Bumped timeout to 60 seconds. Signed-off-by: ebadiere <[email protected]> fix: removed only. Signed-off-by: ebadiere <[email protected]> fix: Added empty line. Signed-off-by: ebadiere <[email protected]> Update packages/server/src/index.ts Co-authored-by: Logan Nguyen <[email protected]> Signed-off-by: Eric Badiere <[email protected]> fix: Applied feedback Signed-off-by: ebadiere <[email protected]> fix: Added back the chai. Signed-off-by: ebadiere <[email protected]> fix: Restored the chai exclude Signed-off-by: ebadiere <[email protected]> fix: Removed the async in the sendJsonRpcRequestWithDelay Signed-off-by: ebadiere <[email protected]> fix: Reduced the timeout to allow tests to finish. Signed-off-by: ebadiere <[email protected]> fix: Moved timeout test to rateLimiter. Signed-off-by: ebadiere <[email protected]> fix: Removed comma after LIMIT_DURATION Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout delay to ensure it works in CI. Signed-off-by: ebadiere <[email protected]> fix: Testing in CI. Removed the .only Signed-off-by: ebadiere <[email protected]> fix: Adjusted the test delay. Signed-off-by: ebadiere <[email protected]> fix: Added error details. Signed-off-by: ebadiere <[email protected]> fix: Added more debug info. Signed-off-by: ebadiere <[email protected]> fix: Added delay for linux. Debugging in CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Setting test timeout Signed-off-by: ebadiere <[email protected]> fix: Debugging in CI. Nested promise. Signed-off-by: ebadiere <[email protected]> fix: Including delay in testfile. Debugging CI. Signed-off-by: ebadiere <[email protected]> fix: Debugging in CIfix: Debugging in CI. Updated package-log. Signed-off-by: ebadiere <[email protected]> fix: Extracted setting logic to util function to be used in server startup and test framework Signed-off-by: ebadiere <[email protected]> fix: Bumped up the timeout for tests on local node. Signed-off-by: ebadiere <[email protected]> * fix: Moved into its own CI job. Signed-off-by: ebadiere <[email protected]> * fix: Cleanup Signed-off-by: ebadiere <[email protected]> * fix: Clean up. Signed-off-by: ebadiere <[email protected]> --------- Signed-off-by: Nana Essilfie-Conduah <[email protected]> Signed-off-by: ebadiere <[email protected]> Co-authored-by: ebadiere <[email protected]>
feat: Add Koa Server Request Timeout (#2504) * Add Koa Server Request Timeout * fix: Added test and documentation to the server request timeout setting. fix: Added doc updated. fix: Bumped timeout to 60 seconds. fix: removed only. fix: Added empty line. Update packages/server/src/index.ts fix: Applied feedback fix: Added back the chai. fix: Restored the chai exclude fix: Removed the async in the sendJsonRpcRequestWithDelay fix: Reduced the timeout to allow tests to finish. fix: Moved timeout test to rateLimiter. fix: Removed comma after LIMIT_DURATION fix: Bumped up the timeout delay to ensure it works in CI. fix: Testing in CI. Removed the .only fix: Adjusted the test delay. fix: Added error details. fix: Added more debug info. fix: Added delay for linux. Debugging in CI. fix: Debugging in CI. Setting test timeout fix: Debugging in CI. Nested promise. fix: Including delay in testfile. Debugging CI. fix: Debugging in CIfix: Debugging in CI. Updated package-log. fix: Extracted setting logic to util function to be used in server startup and test framework fix: Bumped up the timeout for tests on local node. * fix: Moved into its own CI job. * fix: Cleanup * fix: Clean up. --------- Signed-off-by: Nana Essilfie-Conduah <[email protected]> Signed-off-by: ebadiere <[email protected]> Signed-off-by: Logan Nguyen <[email protected]> Co-authored-by: Nana Essilfie-Conduah <[email protected]> Co-authored-by: ebadiere <[email protected]>
Description:
Add Koa Server Request Timeout
Related issue(s):
Fixes #2503
Notes for reviewer:
Checklist