Mute list fallback to cache if timed out #5269
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -92,7 +92,9 @@ class LLDispatchEmptyMuteList : public LLDispatchHandler | |||||||
| const LLUUID& invoice, | ||||||||
| const sparam_t& strings) | ||||||||
| { | ||||||||
| LLMuteList* mute_list = LLMuteList::getInstance(); | ||||||||
| mute_list->clearCachedMutes(); | ||||||||
| mute_list->setLoaded(LLMuteList::MLS_SERVER_EMPTY); | ||||||||
| return true; | ||||||||
| } | ||||||||
| }; | ||||||||
|
|
@@ -155,7 +157,9 @@ std::string LLMute::getDisplayType() const | |||||||
| //----------------------------------------------------------------------------- | ||||||||
| LLMuteList::LLMuteList() : | ||||||||
| mLoadState(ML_INITIAL), | ||||||||
| mLoadSource(MLS_NONE), | ||||||||
| mRequestStartTime(0.f), | ||||||||
| mTriedCacheFallback(false) | ||||||||
| { | ||||||||
| gGenericDispatcher.addHandler("emptymutelist", &sDispatchEmptyMuteList); | ||||||||
|
|
||||||||
|
|
@@ -210,9 +214,9 @@ bool LLMuteList::isLinden(const std::string& name) | |||||||
| return last_name == "linden"; | ||||||||
| } | ||||||||
|
|
||||||||
| bool LLMuteList::updateLoadState() | ||||||||
| { | ||||||||
| if (isLoaded() || isFailed()) | ||||||||
| { | ||||||||
| return true; | ||||||||
| } | ||||||||
|
|
@@ -221,9 +225,81 @@ bool LLMuteList::getLoadFailed() const | |||||||
| constexpr F64 WAIT_SECONDS = 30; | ||||||||
| if (mRequestStartTime + WAIT_SECONDS < LLTimer::getTotalSeconds()) | ||||||||
| { | ||||||||
| LL_WARNS() << "Mute list request timed out; trying cache fallback once" << LL_ENDL; | ||||||||
| tryLoadCacheFallback(gAgent.getID(), "request timeout"); | ||||||||
| return isLoaded() || isFailed(); | ||||||||
| } | ||||||||
| } | ||||||||
| return false; | ||||||||
|
Comment on lines
231
to
+247
There was a problem hiding this comment. The PR title/description mentions retrying mute list requests on region change with a short cooldown, but updateLoadState() currently only performs a one-time cache fallback after a fixed timeout and does not trigger any resend/retry behavior on region change. If retries on region transition are still intended, the retry hook/timer logic seems to be missing; otherwise consider updating the PR title/description to avoid implying behavior that isn't present.
Contributor
Author
There was a problem hiding this comment. Restarting the state machine on region change is still on the table, if maintainers think this is worthwhile still I'd be happy to restore this to the concept. I omitted this from this reimplementation to lighten the load/diff, and leave the real fix to the server team as was noted in the existing comments in code. I should've changed the PR title to reflect that, and will. I forget I can do that 😆
Contributor
There was a problem hiding this comment. Is there a chance a rerequest on a region change will fix the issue? From my observation it seemed like if server did not respond, it will keep it that way.
Contributor
Author
There was a problem hiding this comment. Yes there is actually a chance a server request retry on region change could yield a successful request response. I have observed this to actually work a while back, during the few times I was able to reproduce this issue for testing. This would would add a form of coverage for the currently unaddressed case of a fresh login without a cache being unlucky and also not getting a successful mute list response. I do agree though, if the login simulator did not respond it is not likely to do so if retried. I think to be fruitful this would only be attempted on a region change. I recognize that at this point it's not a lot of extra work to wire back up a basic callback on region change to send another request to the new region, but I don't see the point of doing it every time. Only one extra request should do because in the reports of this that I've seen, this issue is often most prominent when logging into a busy/crowded region. If the login simulator request fails, AND the next region also fails, it's likely there's something else going on and we're not going to get anything out of repeated attempts past that. |
||||||||
| } | ||||||||
|
|
||||||||
| void LLMuteList::clearCachedMutes() | ||||||||
| { | ||||||||
| mMutes.clear(); | ||||||||
| mLegacyMutes.clear(); | ||||||||
| } | ||||||||
|
|
||||||||
| const char* LLMuteList::sourceToString(EMuteListSource source) | ||||||||
| { | ||||||||
| switch (source) | ||||||||
| { | ||||||||
| case MLS_NONE: | ||||||||
| return "none"; | ||||||||
| case MLS_SERVER: | ||||||||
| return "server"; | ||||||||
| case MLS_SERVER_EMPTY: | ||||||||
| return "server-empty"; | ||||||||
| case MLS_SERVER_CACHE: | ||||||||
| return "server-cached"; | ||||||||
| case MLS_FALLBACK_CACHE: | ||||||||
| return "fallback-cache"; | ||||||||
| default: | ||||||||
| return "unknown"; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| std::string LLMuteList::getCacheFilename(const LLUUID& agent_id) const | ||||||||
| { | ||||||||
| std::string agent_id_string; | ||||||||
| agent_id.toString(agent_id_string); | ||||||||
| return gDirUtilp->getExpandedFilename(LL_PATH_CACHE, agent_id_string) + ".cached_mute"; | ||||||||
| } | ||||||||
|
|
||||||||
| void LLMuteList::setFailed(const std::string& reason) | ||||||||
| { | ||||||||
| mLoadState = ML_FAILED; | ||||||||
| if (mLoadSource == MLS_NONE) | ||||||||
| { | ||||||||
| LL_WARNS() << "Mute list unavailable: " << reason << LL_ENDL; | ||||||||
| } | ||||||||
| else | ||||||||
| { | ||||||||
| LL_WARNS() << "Mute list unavailable: " << reason << " (last source=" << sourceToString(mLoadSource) << ")" << LL_ENDL; | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| bool LLMuteList::tryLoadCacheFallback(const LLUUID& agent_id, const std::string& reason) | ||||||||
| { | ||||||||
| if (mTriedCacheFallback) | ||||||||
| { | ||||||||
| if (!isLoaded()) | ||||||||
| { | ||||||||
| setFailed("cache fallback already attempted before " + reason); | ||||||||
| } | ||||||||
| return isLoaded(); | ||||||||
| } | ||||||||
|
|
||||||||
| mTriedCacheFallback = true; | ||||||||
| const std::string filename = getCacheFilename(agent_id); | ||||||||
| LL_INFOS() << "Trying mute list cache fallback due to " << reason << ": " << filename << LL_ENDL; | ||||||||
|
|
||||||||
| if (loadFromFile(filename, MLS_FALLBACK_CACHE)) | ||||||||
| { | ||||||||
| LL_WARNS() << "Loaded mute list from cache fallback due to " << reason << LL_ENDL; | ||||||||
| return true; | ||||||||
| } | ||||||||
akleshchev marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
|
|
||||||||
| setFailed("cache fallback failed after " + reason); | ||||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -580,25 +656,28 @@ std::vector<LLMute> LLMuteList::getMutes() const | |||||||
| //----------------------------------------------------------------------------- | ||||||||
| // loadFromFile() | ||||||||
| //----------------------------------------------------------------------------- | ||||||||
| bool LLMuteList::loadFromFile(const std::string& filename, EMuteListSource source) | ||||||||
| { | ||||||||
| LL_PROFILE_ZONE_SCOPED; | ||||||||
|
|
||||||||
| if(!filename.size()) | ||||||||
| { | ||||||||
| LL_WARNS() << "Mute List Filename is Empty!" << LL_ENDL; | ||||||||
| setFailed("empty filename"); | ||||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
| LLFILE* fp = LLFile::fopen(filename, "rb"); /*Flawfinder: ignore*/ | ||||||||
| if (!fp) | ||||||||
| { | ||||||||
| LL_WARNS() << "Couldn't open mute list " << filename << LL_ENDL; | ||||||||
| setFailed("cannot open " + filename); | ||||||||
| return false; | ||||||||
| } | ||||||||
|
|
||||||||
| // Replace previous server-backed state so fallback can be superseded by authoritative data. | ||||||||
| clearCachedMutes(); | ||||||||
|
|
||||||||
| // *NOTE: Changing the size of these buffers will require changes | ||||||||
| // in the scanf below. | ||||||||
| char id_buffer[MAX_STRING]; /*Flawfinder: ignore*/ | ||||||||
|
|
@@ -627,7 +706,7 @@ bool LLMuteList::loadFromFile(const std::string& filename) | |||||||
| } | ||||||||
| } | ||||||||
| fclose(fp); | ||||||||
| setLoaded(source); | ||||||||
|
|
||||||||
| // server does not maintain up-to date account names (not display names!) | ||||||||
| // in this list, so it falls to viewer. | ||||||||
|
|
@@ -737,12 +816,11 @@ bool LLMuteList::isMuted(const std::string& username, U32 flags) const | |||||||
| //----------------------------------------------------------------------------- | ||||||||
| void LLMuteList::requestFromServer(const LLUUID& agent_id) | ||||||||
| { | ||||||||
| const std::string filename = getCacheFilename(agent_id); | ||||||||
| LLCRC crc; | ||||||||
| crc.update(filename); | ||||||||
| mTriedCacheFallback = false; | ||||||||
| mLoadSource = MLS_NONE; | ||||||||
|
|
||||||||
| LLMessageSystem* msg = gMessageSystem; | ||||||||
| msg->newMessageFast(_PREHASH_MuteListRequest); | ||||||||
|
|
@@ -755,17 +833,19 @@ void LLMuteList::requestFromServer(const LLUUID& agent_id) | |||||||
| if (gDisconnected) | ||||||||
| { | ||||||||
| LL_WARNS() << "Trying to request mute list when disconnected!" << LL_ENDL; | ||||||||
| // Guard against potentially writing back to disk since we're not recovering our connection | ||||||||
| mLoadState = ML_LOADED; | ||||||||
| mLoadSource = MLS_FALLBACK_CACHE; | ||||||||
|
Comment on lines
+858
to
+859
There was a problem hiding this comment. In the gDisconnected path, requestFromServer() marks the mute list as ML_LOADED with source=MLS_FALLBACK_CACHE, but it never actually loads the cached mute list. This can leave the session with an empty mute list even when a cache file exists, which contradicts the PR goal of avoiding an empty list. Consider calling tryLoadCacheFallback(agent_id, "disconnected") (or otherwise loading the cache) instead of forcing ML_LOADED here.
Suggested change
Contributor
Author
There was a problem hiding this comment. By setting the load state as loaded and source as cache, we're putting the state of the list in one that will not change again because it is loaded, and that won't be written to disk from cache(). I believe that calling tryLoadCacheFallback() in this code path unnecessary since we will no longer be handling messages from a server so the block list is no longer important to have in a reliable state. Yes this is don't and say we did situation, but it's a safe lie in this case. |
||||||||
| return; | ||||||||
| } | ||||||||
| if (!gAgent.getRegion()) | ||||||||
| { | ||||||||
| LL_WARNS() << "No region for agent yet, skipping mute list request!" << LL_ENDL; | ||||||||
| tryLoadCacheFallback(agent_id, "no region for request"); | ||||||||
| return; | ||||||||
| } | ||||||||
| mLoadState = ML_REQUESTED; | ||||||||
| mRequestStartTime = LLTimer::getTotalSeconds(); | ||||||||
| // Double amount of retries due to this request happening during busy stage | ||||||||
| // Ideally this should be turned into a capability | ||||||||
| gMessageSystem->sendReliable(gAgent.getRegionHost(), LL_DEFAULT_RELIABLE_RETRIES * 2, true, LL_PING_BASED_TIMEOUT_DUMMY, NULL, NULL); | ||||||||
|
|
@@ -777,15 +857,16 @@ void LLMuteList::requestFromServer(const LLUUID& agent_id) | |||||||
|
|
||||||||
| void LLMuteList::cache(const LLUUID& agent_id) | ||||||||
| { | ||||||||
| // Write to disk even if empty, but never from degraded fallback state. | ||||||||
| if (isLoaded() && mLoadSource != MLS_FALLBACK_CACHE) | ||||||||
| { | ||||||||
| const std::string filename = getCacheFilename(agent_id); | ||||||||
| saveToFile(filename); | ||||||||
| } | ||||||||
| else if (isLoaded()) | ||||||||
| { | ||||||||
| LL_WARNS() << "Skipping mute list cache write from fallback-only state" << LL_ENDL; | ||||||||
| } | ||||||||
akleshchev marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||
| } | ||||||||
|
|
||||||||
| //----------------------------------------------------------------------------- | ||||||||
|
|
@@ -812,7 +893,7 @@ void LLMuteList::processMuteListUpdate(LLMessageSystem* msg, void**) | |||||||
|
|
||||||||
| LLMuteList* mute_list = getInstance(); | ||||||||
| mute_list->mLoadState = ML_REQUESTED; | ||||||||
| mute_list->mRequestStartTime = LLTimer::getTotalSeconds(); | ||||||||
|
|
||||||||
| // Todo: Based of logs and testing, there is no callback | ||||||||
| // from server if file doesn't exist server side. | ||||||||
|
|
@@ -831,12 +912,7 @@ void LLMuteList::processMuteListUpdate(LLMessageSystem* msg, void**) | |||||||
| void LLMuteList::processUseCachedMuteList(LLMessageSystem* msg, void**) | ||||||||
| { | ||||||||
| LL_INFOS() << "LLMuteList::processUseCachedMuteList()" << LL_ENDL; | ||||||||
| LLMuteList::getInstance()->loadFromFile(LLMuteList::getInstance()->getCacheFilename(gAgent.getID()), MLS_SERVER_CACHE); | ||||||||
| } | ||||||||
|
|
||||||||
| void LLMuteList::onFileMuteList(void** user_data, S32 error_code, LLExtStat ext_status) | ||||||||
|
|
@@ -845,13 +921,13 @@ void LLMuteList::onFileMuteList(void** user_data, S32 error_code, LLExtStat ext_ | |||||||
| if(local_filename_and_path && !local_filename_and_path->empty() && (error_code == 0)) | ||||||||
| { | ||||||||
| LL_INFOS() << "Received mute list from server" << LL_ENDL; | ||||||||
| LLMuteList::getInstance()->loadFromFile(*local_filename_and_path, MLS_SERVER); | ||||||||
| LLFile::remove(*local_filename_and_path); | ||||||||
| } | ||||||||
| else | ||||||||
| { | ||||||||
| LL_INFOS() << "LLMuteList xfer failed with code " << error_code << LL_ENDL; | ||||||||
| LLMuteList::getInstance()->tryLoadCacheFallback(gAgent.getID(), "xfer failure"); | ||||||||
| } | ||||||||
| delete local_filename_and_path; | ||||||||
| } | ||||||||
|
|
@@ -908,9 +984,10 @@ void LLMuteList::removeObserver(LLMuteListObserver* observer) | |||||||
| mObservers.erase(observer); | ||||||||
| } | ||||||||
|
|
||||||||
| void LLMuteList::setLoaded(EMuteListSource source) | ||||||||
| { | ||||||||
| mLoadState = ML_LOADED; | ||||||||
| mLoadSource = source; | ||||||||
|
Contributor
There was a problem hiding this comment. Please add a warns if we are setting ML_LOADED when it was already in the same state. It would be good for diagnosis to see potential 'overwrite' in logs. |
||||||||
| notifyObservers(); | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
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.
Please add comment about the purpose of the 'clear'. That we received list from server, might have something from cache, but server takes priority?