-
Notifications
You must be signed in to change notification settings - Fork 104
Mute list fallback to cache + retry on region change if still pending #5269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -59,12 +59,28 @@ | |
| #include "llworld.h" //for particle system banning | ||
| #include "llimview.h" | ||
| #include "llnotifications.h" | ||
| #include "llnotificationsutil.h" | ||
| #include "llviewercontrol.h" | ||
| #include "llviewerobjectlist.h" | ||
| #include "lltrans.h" | ||
| #include "lleventtimer.h" | ||
|
|
||
| namespace | ||
| { | ||
| constexpr F32 MUTE_LIST_REQUEST_TIMEOUT_SECONDS = 30.f; | ||
| constexpr F32 MUTE_LIST_REQUEST_COOLDOWN_SECONDS = 5.f; | ||
|
|
||
| void notify_mute_list_cache_used() | ||
| { | ||
| static bool s_notified = false; | ||
| if (s_notified) | ||
| { | ||
| return; | ||
| } | ||
| s_notified = true; | ||
| LLNotificationsUtil::add("MuteListFallbackCache"); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this should be shown to a user. At least in some cases it's a bug that needs to be fixed server side in others viewer should rerequest. If viewer timeouts, got an empty list or errors getting the list, you probably can do something about it via MuteCRC field. But I agree that if something got wrong we at least should get the data from cache. Or read cache first (issues with this approach if outdated?), mark as 'no send', apply server response on top... P.S. Related: #4267
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I added a notification because I felt the user impact of potentially missing recent uncached mutes that the server could be aware of that we are not could be highly disruptive to the user. In a perfect world they should never see the notification because the replies to The notification will absolutely be removed if it's decided to not be acceptable. I felt it was appropriate at the time given the potential impact of missing mutes.
That is my thought, because for some unknown reason these messages are being lost or remain unsent at random for some users.
The What I considered when deciding not to go for the cached data first, then layer on the server once (if?) we get it was the merging logic. If we were to load from cache first and later learn of a newer server version, it is not guaranteed to be reconcilable with what we have in our cache. For merging the server list with the cached list, the assumptions I would have to make could potentially result in unintended re-mutes or unintended unmutes based on which we determine to be the correct state of a mute that is present vs not or different in one list compared to the other. I was very anxious to repeatedly request a mute list from the region because I do not want to generate undue load with loops. There are two timeouts/cooldowns in place within my PR
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I will create a server ticket for that. I know at least one case with a repro where server isn't responding yet should.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Makes sense. But region change can end up requesting indefinetely either way. Better add some kind of retry limit there.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
SendReliable supports callbacks. Might be possible to refine this by detecting send failures to log better and to rerequest on failures. |
||
| } | ||
|
|
||
| // This method is used to return an object to mute given an object id. | ||
| // Its used by the LLMute constructor and LLMuteList::isMuted. | ||
| LLViewerObject* get_object_to_mute_from_id(LLUUID object_id) | ||
|
|
@@ -155,7 +171,8 @@ std::string LLMute::getDisplayType() const | |
| //----------------------------------------------------------------------------- | ||
| LLMuteList::LLMuteList() : | ||
| mLoadState(ML_INITIAL), | ||
| mRequestStartTime(0.f) | ||
| mRequestStartTime(0.f), | ||
| mRequestTimeout(nullptr) | ||
| { | ||
| gGenericDispatcher.addHandler("emptymutelist", &sDispatchEmptyMuteList); | ||
|
|
||
|
|
@@ -178,6 +195,8 @@ LLMuteList::LLMuteList() : | |
| // but this way is just more convinient | ||
| onAccountNameChanged(id, av_name.getUserName()); | ||
| }); | ||
|
|
||
| mRegionChangedSlot = gAgent.addRegionChangedCallback([this]() { onRegionChanged(); }); | ||
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
|
|
@@ -191,6 +210,11 @@ LLMuteList::~LLMuteList() | |
| void LLMuteList::cleanupSingleton() | ||
| { | ||
| LLAvatarNameCache::getInstance()->setAccountNameChangedCallback(nullptr); | ||
| if (mRegionChangedSlot.connected()) | ||
| { | ||
| mRegionChangedSlot.disconnect(); | ||
| } | ||
| cancelRequestTimeout(); | ||
| } | ||
|
|
||
| bool LLMuteList::isLinden(const std::string& name) | ||
|
|
@@ -735,6 +759,17 @@ bool LLMuteList::isMuted(const std::string& username, U32 flags) const | |
| //----------------------------------------------------------------------------- | ||
| void LLMuteList::requestFromServer(const LLUUID& agent_id) | ||
| { | ||
| if (mLoadState == ML_REQUESTED) | ||
| { | ||
| const F64 now = LLTimer::getElapsedSeconds(); | ||
| if (now - mRequestStartTime < MUTE_LIST_REQUEST_COOLDOWN_SECONDS) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| cancelRequestTimeout(); | ||
|
|
||
| std::string agent_id_string; | ||
| std::string filename; | ||
| agent_id.toString(agent_id_string); | ||
|
|
@@ -764,11 +799,64 @@ void LLMuteList::requestFromServer(const LLUUID& agent_id) | |
| } | ||
| mLoadState = ML_REQUESTED; | ||
| mRequestStartTime = LLTimer::getElapsedSeconds(); | ||
| const F64 request_start_time = mRequestStartTime; | ||
| mRequestTimeout = LLEventTimer::run_after(MUTE_LIST_REQUEST_TIMEOUT_SECONDS, [this, request_start_time]() | ||
| { | ||
| mRequestTimeout = nullptr; | ||
| if (mLoadState != ML_REQUESTED) | ||
| { | ||
| return; | ||
| } | ||
| if (mRequestStartTime != request_start_time) | ||
| { | ||
| return; | ||
| } | ||
| LL_WARNS() << "Mute list request timed out; loading cached mute list" << LL_ENDL; | ||
| if (!loadFromCache()) | ||
| { | ||
| LL_WARNS() << "Failed to load cached mute list after timeout" << LL_ENDL; | ||
| mLoadState = ML_FAILED; | ||
| } | ||
| else | ||
| { | ||
| notify_mute_list_cache_used(); | ||
| } | ||
| }); | ||
| // 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); | ||
| } | ||
|
|
||
| void LLMuteList::onRegionChanged() | ||
| { | ||
| if (isLoaded()) | ||
| { | ||
| return; | ||
| } | ||
| if (mLoadState == ML_REQUESTED) | ||
| { | ||
| const F64 now = LLTimer::getElapsedSeconds(); | ||
| if (now - mRequestStartTime < MUTE_LIST_REQUEST_COOLDOWN_SECONDS) | ||
| { | ||
| return; | ||
| } | ||
| } | ||
| if (gDisconnected || !gAgent.getRegion() || gAgent.getID().isNull()) | ||
| { | ||
| return; | ||
| } | ||
| requestFromServer(gAgent.getID()); | ||
| } | ||
|
|
||
| void LLMuteList::cancelRequestTimeout() | ||
| { | ||
| if (mRequestTimeout) | ||
| { | ||
| delete mRequestTimeout; | ||
| } | ||
| mRequestTimeout = nullptr; | ||
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| // cache() | ||
| //----------------------------------------------------------------------------- | ||
|
|
@@ -786,6 +874,14 @@ void LLMuteList::cache(const LLUUID& agent_id) | |
| } | ||
| } | ||
|
|
||
| bool LLMuteList::loadFromCache() | ||
| { | ||
| std::string agent_id_string; | ||
| gAgent.getID().toString(agent_id_string); | ||
| std::string filename = gDirUtilp->getExpandedFilename(LL_PATH_CACHE, agent_id_string) + ".cached_mute"; | ||
| return loadFromFile(filename); | ||
| } | ||
|
|
||
| //----------------------------------------------------------------------------- | ||
| // Static message handlers | ||
| //----------------------------------------------------------------------------- | ||
|
|
@@ -825,12 +921,7 @@ void LLMuteList::processMuteListUpdate(LLMessageSystem* msg, void**) | |
| void LLMuteList::processUseCachedMuteList(LLMessageSystem* msg, void**) | ||
| { | ||
| LL_INFOS() << "LLMuteList::processUseCachedMuteList()" << LL_ENDL; | ||
|
|
||
| std::string agent_id_string; | ||
| gAgent.getID().toString(agent_id_string); | ||
| std::string filename; | ||
| filename = gDirUtilp->getExpandedFilename(LL_PATH_CACHE,agent_id_string) + ".cached_mute"; | ||
| LLMuteList::getInstance()->loadFromFile(filename); | ||
| LLMuteList::getInstance()->loadFromCache(); | ||
| } | ||
|
|
||
| void LLMuteList::onFileMuteList(void** user_data, S32 error_code, LLExtStat ext_status) | ||
|
|
@@ -845,7 +936,16 @@ void LLMuteList::onFileMuteList(void** user_data, S32 error_code, LLExtStat ext_ | |
| } | ||
| else | ||
| { | ||
| LLMuteList::getInstance()->mLoadState = ML_FAILED; | ||
| LL_WARNS() << "Mute list transfer failed; falling back to cached mute list" << LL_ENDL; | ||
| LLMuteList* mute_list = LLMuteList::getInstance(); | ||
| if (!mute_list->loadFromCache()) | ||
| { | ||
| mute_list->mLoadState = ML_FAILED; | ||
| } | ||
| else | ||
| { | ||
| notify_mute_list_cache_used(); | ||
| } | ||
| } | ||
| delete local_filename_and_path; | ||
| } | ||
|
|
@@ -905,6 +1005,7 @@ void LLMuteList::removeObserver(LLMuteListObserver* observer) | |
| void LLMuteList::setLoaded() | ||
| { | ||
| mLoadState = ML_LOADED; | ||
| cancelRequestTimeout(); | ||
| 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 onChange() implementation only handles the case where mMuteListSize transitions from 0 to a positive value. If the mute list size changes in any other way through a batch operation (e.g., from 10 to 15 items when cache loads), the UI will not update because the condition on line 88 will be false. Consider updating mMuteListSize at the end of this method or triggering a refresh whenever current_size != mMuteListSize, not just when transitioning from zero.