Fix/achievements winhttp timeout crash#14311
Fix/achievements winhttp timeout crash#14311salas-dev-max wants to merge 1 commit intoPCSX2:masterfrom
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thank you for submitting a contribution to PCSX2
As this is your first pull request, please be aware of the contributing guidelines.
Additionally, as per recent changes in GitHub Actions, your pull request will need to be approved by a maintainer before GitHub Actions can run against it. You can find more information about this change here.
Please be patient until this happens. In the meantime if you'd like to confirm the builds are passing, you have the option of opening a PR on your own fork, just make sure your fork's master branch is up to date!
|
@salas-dev-max some windows CI are failing :) |
Darn |
F0bes
left a comment
F0bes
left a comment
There was a problem hiding this comment.
Initial feedback, not tested.
Squash your commits. Commits should start capitalized.
| Console.Error("WinHttpSendRequest() failed: %u", GetLastError()); | ||
| req->status_code = HTTP_STATUS_ERROR; | ||
| req->state.store(Request::State::Complete); | ||
| Console.Error("WinHttpSendRequest() failed: %u", GetLastError()); |
| DevCon.WriteLn("Started HTTP request for '%s'", req->url.c_str()); | ||
| req->state = Request::State::Started; | ||
| req->start_time = Common::Timer::GetCurrentValue(); | ||
| } |
There was a problem hiding this comment.
If we've failed, should we not return false?
| RC_API_SERVER_RESPONSE_CLIENT_ERROR : | ||
| RC_API_SERVER_RESPONSE_RETRYABLE_CLIENT_ERROR) | ||
| : status_code; | ||
| RC_API_SERVER_RESPONSE_CLIENT_ERROR : |
| RC_API_SERVER_RESPONSE_RETRYABLE_CLIENT_ERROR) | ||
| : status_code; | ||
| RC_API_SERVER_RESPONSE_CLIENT_ERROR : | ||
| status_code == HTTPDownloader::HTTP_STATUS_TIMEOUT ? |
There was a problem hiding this comment.
I'd rather use some temporaries to make it more readable.
Something like this, or even some if statements is more readable imo
const bool is_error = (status_code <= 0);
const bool is_retryable =
is_error && (status_code == HTTPDownloader::HTTP_STATUS_TIMEOUT);
rr.http_status_code =
is_error
? (is_retryable
? RC_API_SERVER_RESPONSE_RETRYABLE_CLIENT_ERROR
: RC_API_SERVER_RESPONSE_CLIENT_ERROR)
: status_code;- Fix state overwrite in StartRequest() where Complete was clobbered by Started when WinHttpSendRequest failed synchronously - Set explicit WinHTTP timeouts (15s) shorter than app-level timeout to eliminate the polling/callback race on slow connections - Map HTTP_STATUS_ERROR to CLIENT_ERROR (non-retryable) to prevent rc_client retry loops on hard WinHTTP failures (error 12002/timeout) Closes PCSX2#14277
salas-dev-max
force-pushed
the
fix/achievements-winhttp-timeout-crash
branch
from
a850ea2 to
2500888
Compare
3 participants
Description of Changes
Fix crash when RetroAchievements server is unreachable due to a censored or blocking network connection. Affects any network that hangs connections rather than refusing them (error 12002 /
ERROR_WINHTTP_TIMEOUT).Rationale behind Changes
Three bugs in the WinHTTP layer caused a crash instead of a graceful failure:
State overwrite in
StartRequest()— whenWinHttpSendRequestfails synchronously, the request state was correctly set toCompletebut then immediately overwritten withStartedunconditionally. The async callback never fires for a failed send, so the request hung until the app-level timeout.Competing timeouts — WinHTTP's default receive timeout and the app-level timeout are both 30s, creating a race between the WinHTTP callback thread and the polling thread. Explicit WinHTTP timeouts (15s) ensure the async error always fires before the poll loop races it.
Incorrect error mapping in
ClientServerCall()—HTTP_STATUS_ERRORwas mapped toRC_API_SERVER_RESPONSE_RETRYABLE_CLIENT_ERROR, causing rc_client to retry indefinitely on hard WinHTTP failures. On a network that consistently hangs connections this retry loop leads to a crash. Fixed by mapping it toCLIENT_ERROR(non-retryable).Closes #14277
Suggested Testing Steps
Add
127.0.0.1 retroachievements.orgto your hosts file, launch PCSX2 with achievements enabled, and verify it logs an error and continues running instead of crashing after ~30 seconds.Did you use AI to help find, test, or implement this issue or feature?
Yes. Used Claude to help trace the root cause through the WinHTTP callback chain and identify the three contributing bugs. All changes were reviewed and understood before submission.