fix: add default 30s timeout to notification provider HTTP requests#7313
fix: add default 30s timeout to notification provider HTTP requests#7313andymboyle wants to merge 1 commit intolouislam:masterfrom
Conversation
|
Hello and thanks for lending a paw to Uptime Kuma! 🐻👋 |
|
Hello! This pull request does not follow the repository's PR template and is being closed automatically. |
There was a problem hiding this comment.
Pull request overview
Adds a default request timeout for all notification-provider HTTP calls to prevent indefinite hangs when upstream APIs accept a connection but never respond.
Changes:
- Set a default 30s
timeoutinsidegetAxiosConfigWithProxy()when no timeout is provided.
| * @returns {object} Axios config | ||
| */ | ||
| getAxiosConfigWithProxy(axiosConfig = {}) { | ||
| if (!axiosConfig.timeout) { |
There was a problem hiding this comment.
The default-timeout guard uses a falsy check (!axiosConfig.timeout), which will override an explicitly configured timeout: 0 (Axios uses 0 to mean “no timeout”). To preserve provider overrides, check specifically for undefined (or == null if you also want to treat null as “unset”) before applying the 30s default.
| if (!axiosConfig.timeout) { | |
| if (axiosConfig.timeout === undefined) { |
Look through our codebase more carfully. We have a timeout mechanism. And it is not 30s.. |
Appreciate the feedback—looks like I have a lot further to go with my static analyzer tool before I try to file any PRs. Thank you! |
Howdy! So I'm working on a static analysis tool that checks for missing timeouts and fixes them, and I ran it against the Uptime Kuma codebase. It flagged that none of the notification providers set a timeout on their HTTP requests, so if a notification API accepts the connection but hangs on the response, the request blocks indefinitely. (Maybe this is too small to care about, but hey, the tool found it and was able to fix it automatically, so I'm kind of checking to prove that it's useful.)
This is just a one-line fix in
getAxiosConfigWithProxy()that adds a default 30-second timeout to all notification provider HTTP requests. Since every provider already calls this function, it covers all of them without touching any individual provider files.The
if (!axiosConfig.timeout)check means any provider that explicitly sets its own timeout will keep it. Basically it's just adding a default where none exists.Totally understand if this is too small to be worth the review time. (Also just wanted to see if my little tool is actually finding useful stuff, so thanks for humoring me either way!) Cheers!