matthew/The-Powder-Toy
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fix requests not being marked done when they fail early

Browse Source 
They were marked done as far as Libcurl.cpp semantics went, but they weren't noticed by the Worker loop and thus weren't actually marked done with MarkDone.

Broken by 0cc179ae4e where I quietly (i.e. no mention of this in the commit message >_>) replaced curl_multi_wait with curl_multi_poll, the former of which would return after some time even if nothing happened and the latter of which doesn't. Thus, the request manager loop kept iterating, if slowly, and masked this issue: these half-baked requests ended up being noticed in the iteration of the Worker loop that came after the one during which they were marked done.

Also fix the MotD being finalized improperly in Client.
This commit is contained in:
Tamás Bálint Misius 2024-01-01 17:08:11 +01:00
parent 70f6b68fc5
commit 4a99004327
No known key found for this signature in database
GPG Key ID: 5B472A12F6ECA9F2
2 changed files with 17 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/client/Client.cpp
View File

@ -131,7 +131,7 @@ void Client::Tick()
{ {
updateInfo = info.updateInfo; updateInfo = info.updateInfo;
applyUpdateInfo = true; applyUpdateInfo = true;
messageOfTheDay = info.messageOfTheDay; SetMessageOfTheDay(info.messageOfTheDay);
} }
for (auto &notification : info.notifications) for (auto &notification : info.notifications)
{ {
@ -142,7 +142,7 @@ void Client::Tick()
{ {
if (!usingAltUpdateServer) if (!usingAltUpdateServer)
{ {
messageOfTheDay = ByteString::Build("Error while fetching MotD: ", ex.what()).FromUtf8(); SetMessageOfTheDay(ByteString::Build("Error while fetching MotD: ", ex.what()).FromUtf8());
} }
} }
versionCheckRequest.reset(); versionCheckRequest.reset();
@ -154,7 +154,7 @@ void Client::Tick()
auto info = alternateVersionCheckRequest->Finish(); auto info = alternateVersionCheckRequest->Finish();
updateInfo = info.updateInfo; updateInfo = info.updateInfo;
applyUpdateInfo = true; applyUpdateInfo = true;
messageOfTheDay = info.messageOfTheDay; SetMessageOfTheDay(info.messageOfTheDay);
for (auto &notification : info.notifications) for (auto &notification : info.notifications)
{ {
AddServerNotification(notification); AddServerNotification(notification);
@ -162,7 +162,7 @@ void Client::Tick()
} }
catch (const http::RequestError &ex) catch (const http::RequestError &ex)
{ {
messageOfTheDay = ByteString::Build("Error while checking for updates: ", ex.what()).FromUtf8(); SetMessageOfTheDay(ByteString::Build("Error while checking for updates: ", ex.what()).FromUtf8());
} }
alternateVersionCheckRequest.reset(); alternateVersionCheckRequest.reset();
} }

20
src/client/http/requestmanager/Libcurl.cpp
View File

@ -296,13 +296,8 @@ namespace http
{ {
{ {
std::lock_guard lk(sharedStateMx); std::lock_guard lk(sharedStateMx);
for (auto &requestHandle : requestHandles) // Register new handles first. This always succeeds even if the handle is "failed early" so that
{ // a single MarkDone call could be issued on all handles further down in this block.
if (requestHandle->statusCode)
{
requestHandlesToUnregister.push_back(requestHandle);
}
}
for (auto &requestHandle : requestHandlesToRegister) for (auto &requestHandle : requestHandlesToRegister)
{ {
// Must not be present // Must not be present
@ -311,6 +306,17 @@ namespace http
RegisterRequestHandle(requestHandle); RegisterRequestHandle(requestHandle);
} }
requestHandlesToRegister.clear(); requestHandlesToRegister.clear();
// Then unregister done handles. As explained above, registering a new handle may also immediately mark
// it done and we won't be coming back here until Wait() returns, so this has to come second.
for (auto &requestHandle : requestHandles)
{
if (requestHandle->statusCode)
{
requestHandlesToUnregister.push_back(requestHandle);
}
}
// Actually unregister handles queued to be unregistered. They can be queued just above, or from another thread.
// Thus, it's ok for them to be in the queue multiple times, but it's not ok to try to unregister them multiple times.
for (auto &requestHandle : requestHandlesToUnregister) for (auto &requestHandle : requestHandlesToUnregister)
{ {
auto eraseFrom = std::remove(requestHandles.begin(), requestHandles.end(), requestHandle); auto eraseFrom = std::remove(requestHandles.begin(), requestHandles.end(), requestHandle);