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,7 @@ class LLDispatchEmptyMuteList : public LLDispatchHandler | |
| const LLUUID& invoice, | ||
| const sparam_t& strings) | ||
| { | ||
| LLMuteList::getInstance()->setLoaded(LLMuteList::MLS_SERVER_EMPTY); | ||
| return true; | ||
| } | ||
| }; | ||
|
|
@@ -155,7 +155,9 @@ std::string LLMute::getDisplayType() const | |
| //----------------------------------------------------------------------------- | ||
| LLMuteList::LLMuteList() : | ||
| mLoadState(ML_INITIAL), | ||
| mLoadSource(MLS_NONE), | ||
| mRequestStartTime(0.f), | ||
| mTriedCacheFallback(false) | ||
| { | ||
| gGenericDispatcher.addHandler("emptymutelist", &sDispatchEmptyMuteList); | ||
|
|
||
|
|
@@ -210,7 +212,7 @@ bool LLMuteList::isLinden(const std::string& name) | |
| return last_name == "linden"; | ||
| } | ||
|
|
||
| bool LLMuteList::getLoadFailed() | ||
| { | ||
| if (mLoadState == ML_FAILED) | ||
| { | ||
|
|
@@ -221,12 +223,78 @@ 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 mLoadState == ML_FAILED; | ||
| } | ||
| } | ||
| 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. |
||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| static LLVOAvatar* find_avatar(const LLUUID& id) | ||
| { | ||
| LLViewerObject *obj = gObjectList.findObject(id); | ||
|
|
@@ -580,25 +648,29 @@ 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. | ||
| mMutes.clear(); | ||
| mLegacyMutes.clear(); | ||
|
|
||
| // *NOTE: Changing the size of these buffers will require changes | ||
| // in the scanf below. | ||
| char id_buffer[MAX_STRING]; /*Flawfinder: ignore*/ | ||
|
|
@@ -627,7 +699,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 +809,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 +826,17 @@ void LLMuteList::requestFromServer(const LLUUID& agent_id) | |
| if (gDisconnected) | ||
| { | ||
| LL_WARNS() << "Trying to request mute list when disconnected!" << LL_ENDL; | ||
| tryLoadCacheFallback(agent_id, "disconnected before request"); | ||
akleshchev marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| 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 +848,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 +884,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 +903,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 +912,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 +975,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.
The "emptymutelist" dispatch now only marks the list as loaded (source=server-empty) but does not clear any existing entries. If a cache fallback was loaded earlier (e.g., after a request timeout) and the server message arrives late, this will incorrectly keep stale cached mutes even though the server is explicitly saying the list is empty. Consider clearing mMutes/mLegacyMutes (and possibly writing an empty cache) before calling setLoaded(MLS_SERVER_EMPTY).
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.
Makes sense. There is a case where user already did some changes this session, but probably not worth accounting for.
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.
Agreed on not worth accounting for, I operate on the belief that add()/remove() calls made in a degraded state still get received and accounted for by the simulator in lieu of maintaining some kind of clean/dirty transactional mess like I had thought up prior.