Repository navigation
Version check: bound the UI-thread download with an overall timeout - #1592
Merged
Merged
Conversation
Clicking Settings (and program stop/startup, Main.cpp) calls check_new_version() on the UI thread, which downloads LatestVersion.json synchronously. QNetworkRequest::setTransferTimeout(5s) only limits data transfer, not the connection phase, and the overall watchdog in download_file() is disabled (#if 0), so a connection that stalls to raw.githubusercontent.com blocks QEventLoop::exec() - and the UI thread - indefinitely (the app appears to hang when Settings is clicked). Add an optional overall deadline to download_file()/download_json_file() (default: no limit, so resource downloads keep their current behavior) and give the version check a 10 second budget; on expiry the reply is aborted and the existing OperationFailedException path skips the check.
Mysticial
reviewed
Oct 15, 2026
| logger, | ||
| "https://raw.githubusercontent.com/PokemonAutomation/ComputerControl/master/LatestVersion.json" | ||
| "https://raw.githubusercontent.com/PokemonAutomation/ComputerControl/master/LatestVersion.json", | ||
| std::chrono::seconds(10) |
Collaborator
There was a problem hiding this comment.
I'd reduce this to 3 seconds. Since it's not an important check.
Mysticial
reviewed
Oct 15, 2026
| // data transfer, so without it a connection that stalls would block | ||
| // this call forever). std::chrono::seconds(0) (the default) imposes no | ||
| // overall limit. | ||
| std::string download_file(Logger& logger, const std::string& url, std::chrono::seconds overall_timeout = std::chrono::seconds(0)); |
Collaborator
There was a problem hiding this comment.
line is getting long here. Break it into multiple lines.
Mysticial
reviewed
Oct 15, 2026
| // Throws OperationFailedException if failed to download. | ||
| // Returns empty value if invalid JSON. | ||
| JsonValue download_json_file(Logger& logger, const std::string& url); | ||
| JsonValue download_json_file(Logger& logger, const std::string& url, std::chrono::seconds overall_timeout = std::chrono::seconds(0)); |
Mysticial
reviewed
Oct 15, 2026
| // Deadline for the whole request, including the connection phase | ||
| // (setTransferTimeout() above only limits data transfer). Without | ||
| // this, a connection that stalls would block loop.exec() forever. | ||
| QTimer overall_timer; |
Collaborator
There was a problem hiding this comment.
Not a real fan of QTimer since we're trying to minimize Qt usage. But this file is already littered with it. This is just a comment, you don't need to do anything.
Mysticial
approved these changes
Oct 15, 2026
Collaborator
|
This PR was accidentally generated by Gin's bot (i.e. I wanted it to fix one bug, but it found this bug instead). I'll take a look at your suggestions today or over the next few days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Clicking Settings (and program stop/startup) calls
check_new_version()on the UI thread, which downloads LatestVersion.json synchronously.QNetworkRequest::setTransferTimeout(5s)only limits data transfer, not the connection phase, and the overall watchdog indownload_file()is disabled (#if 0in FileDownloader.cpp), so a connection that stalls to raw.githubusercontent.com blocksQEventLoop::exec()- and the UI thread - indefinitely. Symptom: the app hangs when Settings is clicked.Adds an optional overall deadline to
download_file()/download_json_file()(default: no limit, so resource downloads keep their current behavior) and gives the version check a 10 second budget; on expiry the reply is aborted and the existing OperationFailedException path skips the check.Verified with a full Linux CI-style build (
pa-build).