-
Notifications
You must be signed in to change notification settings - Fork 194
fix: timeout to all database queries #1045
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
base: master
Are you sure you want to change the base?
Conversation
| const originalQueryAsync = hubDB.queryAsync; | ||
| hubDB.queryAsync = function (sql: string, values?: any) { | ||
| return originalQueryAsync.call(this, { | ||
| sql: sql, | ||
| values: values, | ||
| timeout: QUERY_TIMEOUT_MS | ||
| }); | ||
| }; |
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.
Can't you simplify like this?
| const originalQueryAsync = hubDB.queryAsync; | |
| hubDB.queryAsync = function (sql: string, values?: any) { | |
| return originalQueryAsync.call(this, { | |
| sql: sql, | |
| values: values, | |
| timeout: QUERY_TIMEOUT_MS | |
| }); | |
| }; | |
| hubDB.queryAsync = (sql: string, values?: any) => hubDB.queryAsync.call(this, { | |
| sql, | |
| values, | |
| timeout: QUERY_TIMEOUT_MS | |
| }); |
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.
That may break the this actually. But dont need to repeat sql and values, or doing a const for queryAsync
|
There isn't a more native way to do it without rewriting query? Did you find this trick somewhere? |
I found this here https://www.npmjs.com/package/mysql#timeouts https://discord.com/channels/955773041898573854/1403444404295045141/1405865171506696254 If not we may have to pass it to each query individually, so we overwrite queryAsync |
|
I think this doesn't cancel query on DB itself. Is this aimed at just long queries or is there other usecase? Because if we add this kind of timeout it would basically kill long queries on the API while still being an issue on the DB itself (so we just waste resources without giving response to the client). |
|
Hmm yes we need a timeout on DB https://discord.com/channels/955773041898573854/1403444404295045141/1406532788194250872 With this PR, it fixes the earlier |
|
If the goal is to prevent long running connections, how about just adding a timeout on the express js side ? https://expressjs.com/en/resources/middleware/timeout.html |
Nice yea, we should also add some timeout like this, does it also stop network requests/DB connections on timeout? |
Nope, it just releases the resource by closing the http request, which is essentially the same feature as this PR, since it's also not killing the long running query |
Issue
Timeout for quires doesn't work, this keep the connections open for long durations than we expect them to
Summary
Adds automatic 30-second timeout to all database queries to prevent long-running queries from hanging indefinitely.
Changes
queryAsyncmethod inmysql.tsto automatically add timeout to all queriesHow to Test
QUERY_TIMEOUT_MSfrom30000to5000insrc/helpers/mysql.ts