From a8cdcfc9a893b7debf7c006022b57c389b50bf0d Mon Sep 17 00:00:00 2001 From: "Brad Payne (Vir Linden)" Date: Fri, 12 Apr 2013 09:16:25 -0400 Subject: SH-4061 WIP - moved retry policy to newview so it can work with either llmessage or CoreHttp libraries. Updated tests. --- indra/newview/llhttpretrypolicy.h | 94 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100755 indra/newview/llhttpretrypolicy.h (limited to 'indra/newview/llhttpretrypolicy.h') diff --git a/indra/newview/llhttpretrypolicy.h b/indra/newview/llhttpretrypolicy.h new file mode 100755 index 0000000000..ca37e5f73c --- /dev/null +++ b/indra/newview/llhttpretrypolicy.h @@ -0,0 +1,94 @@ +/** + * @file file llhttpretrypolicy.h + * @brief declarations for http retry policy class. + * + * $LicenseInfo:firstyear=2013&license=viewerlgpl$ + * Second Life Viewer Source Code + * Copyright (C) 2013, Linden Research, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; + * version 2.1 of the License only. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + * + * Linden Research, Inc., 945 Battery Street, San Francisco, CA 94111 USA + * $/LicenseInfo$ + */ + +#ifndef LL_RETRYPOLICY_H +#define LL_RETRYPOLICY_H + +#include "lltimer.h" +#include "llthread.h" + +#include "llhttpconstants.h" + +// For compatibility with new core http lib. +#include "httpresponse.h" +#include "httpheaders.h" + +// This is intended for use with HTTP Clients/Responders, but is not +// specifically coupled with those classes. +class LLHTTPRetryPolicy: public LLThreadSafeRefCount +{ +public: + LLHTTPRetryPolicy() {} + virtual ~LLHTTPRetryPolicy() {} + // Call once after an HTTP failure to update state. + virtual void onFailure(S32 status, const LLSD& headers) = 0; + + virtual void onFailure(const LLCore::HttpResponse *response) = 0; + + virtual bool shouldRetry(F32& seconds_to_wait) const = 0; +}; + +// Very general policy with geometric back-off after failures, +// up to a maximum delay, and maximum number of retries. +class LLAdaptiveRetryPolicy: public LLHTTPRetryPolicy +{ +public: + LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries): + mMinDelay(min_delay), + mMaxDelay(max_delay), + mBackoffFactor(backoff_factor), + mMaxRetries(max_retries), + mDelay(min_delay), + mRetryCount(0), + mShouldRetry(true) + { + } + + // virtual + void onFailure(S32 status, const LLSD& headers); + // virtual + void onFailure(const LLCore::HttpResponse *response); + // virtual + bool shouldRetry(F32& seconds_to_wait) const; + +protected: + bool getRetryAfter(const LLSD& headers, F32& retry_header_time); + bool getRetryAfter(const LLCore::HttpHeaders *headers, F32& retry_header_time); + void onFailureCommon(S32 status, bool has_retry_header_time, F32 retry_header_time); + +private: + + F32 mMinDelay; // delay never less than this value + F32 mMaxDelay; // delay never exceeds this value + F32 mBackoffFactor; // delay increases by this factor after each retry, up to mMaxDelay. + U32 mMaxRetries; // maximum number of times shouldRetry will return true. + F32 mDelay; // current default delay. + U32 mRetryCount; // number of times shouldRetry has been called. + LLTimer mRetryTimer; // time until next retry. + bool mShouldRetry; // Becomes false after too many retries, or the wrong sort of status received, etc. +}; + +#endif -- cgit v1.3 From bb237ce15f0a7bc4a3fbffc45b1e4548fd1d2f81 Mon Sep 17 00:00:00 2001 From: "Brad Payne (Vir Linden)" Date: Fri, 12 Apr 2013 17:04:48 -0400 Subject: SH_4061 WIP - retry policy org and tests --- indra/newview/llhttpretrypolicy.cpp | 23 +++++++++++++++++- indra/newview/llhttpretrypolicy.h | 11 +-------- indra/newview/tests/llhttpretrypolicy_test.cpp | 33 +++++++++++++++++++++----- 3 files changed, 50 insertions(+), 17 deletions(-) (limited to 'indra/newview/llhttpretrypolicy.h') diff --git a/indra/newview/llhttpretrypolicy.cpp b/indra/newview/llhttpretrypolicy.cpp index 5c6dabbe99..82f6eab00e 100755 --- a/indra/newview/llhttpretrypolicy.cpp +++ b/indra/newview/llhttpretrypolicy.cpp @@ -28,6 +28,17 @@ #include "llhttpretrypolicy.h" +LLAdaptiveRetryPolicy::LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries): + mMinDelay(min_delay), + mMaxDelay(max_delay), + mBackoffFactor(backoff_factor), + mMaxRetries(max_retries), + mDelay(min_delay), + mRetryCount(0), + mShouldRetry(true) +{ +} + bool LLAdaptiveRetryPolicy::getRetryAfter(const LLSD& headers, F32& retry_header_time) { return (headers.has(HTTP_IN_HEADER_RETRY_AFTER) @@ -77,6 +88,11 @@ void LLAdaptiveRetryPolicy::onFailure(const LLCore::HttpResponse *response) void LLAdaptiveRetryPolicy::onFailureCommon(S32 status, bool has_retry_header_time, F32 retry_header_time) { + if (!mShouldRetry) + { + llinfos << "keep on failing" << llendl; + return; + } if (mRetryCount > 0) { mDelay = llclamp(mDelay*mBackoffFactor,mMinDelay,mMaxDelay); @@ -111,7 +127,12 @@ void LLAdaptiveRetryPolicy::onFailureCommon(S32 status, bool has_retry_header_ti bool LLAdaptiveRetryPolicy::shouldRetry(F32& seconds_to_wait) const { - llassert(mRetryCount>0); // have to call onFailure() before shouldRetry() + if (mRetryCount == 0) + { + // Called shouldRetry before any failure. + seconds_to_wait = F32_MAX; + return false; + } seconds_to_wait = mShouldRetry ? mRetryTimer.getRemainingTimeF32() : F32_MAX; return mShouldRetry; } diff --git a/indra/newview/llhttpretrypolicy.h b/indra/newview/llhttpretrypolicy.h index ca37e5f73c..6f63f047de 100755 --- a/indra/newview/llhttpretrypolicy.h +++ b/indra/newview/llhttpretrypolicy.h @@ -56,16 +56,7 @@ public: class LLAdaptiveRetryPolicy: public LLHTTPRetryPolicy { public: - LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries): - mMinDelay(min_delay), - mMaxDelay(max_delay), - mBackoffFactor(backoff_factor), - mMaxRetries(max_retries), - mDelay(min_delay), - mRetryCount(0), - mShouldRetry(true) - { - } + LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries); // virtual void onFailure(S32 status, const LLSD& headers); diff --git a/indra/newview/tests/llhttpretrypolicy_test.cpp b/indra/newview/tests/llhttpretrypolicy_test.cpp index 39bd15d62f..43fc1178cc 100755 --- a/indra/newview/tests/llhttpretrypolicy_test.cpp +++ b/indra/newview/tests/llhttpretrypolicy_test.cpp @@ -45,8 +45,12 @@ void RetryPolicyTestObject::test<1>() LLSD headers; F32 wait_seconds; + // No retry until we've finished a try. + ensure("never retry 0", !never_retry.shouldRetry(wait_seconds)); + + // 0 retries max. never_retry.onFailure(500,headers); - ensure("never retry", !never_retry.shouldRetry(wait_seconds)); + ensure("never retry 1", !never_retry.shouldRetry(wait_seconds)); } template<> template<> @@ -70,6 +74,9 @@ void RetryPolicyTestObject::test<3>() bool should_retry; U32 frac_bits = 6; + // No retry until we've finished a try. + ensure("basic_retry 0", !basic_retry.shouldRetry(wait_seconds)); + // Starting wait 1.0 basic_retry.onFailure(500,headers); should_retry = basic_retry.shouldRetry(wait_seconds); @@ -224,10 +231,13 @@ void RetryPolicyTestObject::test<7>() time_t nowseconds; time(&nowseconds); sd_headers[HTTP_IN_HEADER_RETRY_AFTER] = LLDate((F64)nowseconds).asRFC1123(); - LLAdaptiveRetryPolicy policy(17.0,644.0,3.0,10); + LLAdaptiveRetryPolicy policy(17.0,644.0,3.0,5); F32 seconds_to_wait; bool should_retry; + // No retry until we've finished a try. + ensure("header 0", !policy.shouldRetry(seconds_to_wait)); + // no retry header, use default. policy.onFailure(500,LLSD()); should_retry = policy.shouldRetry(seconds_to_wait); @@ -246,7 +256,7 @@ void RetryPolicyTestObject::test<7>() LLCore::HttpHeaders *headers = new LLCore::HttpHeaders(); response->setStatus(503); response->setHeaders(headers); - headers->mHeaders.push_back("retry-after: 600"); + headers->mHeaders.push_back(HTTP_IN_HEADER_RETRY_AFTER + ": 600"); policy.onFailure(response); should_retry = policy.shouldRetry(seconds_to_wait); ensure("header 3",should_retry); @@ -262,13 +272,24 @@ void RetryPolicyTestObject::test<7>() response->setHeaders(headers); LLSD sd_headers; time(&nowseconds); - headers->mHeaders.push_back("retry-after: " + LLDate((F64)nowseconds).asRFC1123()); + headers->mHeaders.push_back(HTTP_IN_HEADER_RETRY_AFTER + ": " + LLDate((F64)nowseconds).asRFC1123()); policy.onFailure(response); should_retry = policy.shouldRetry(seconds_to_wait); - ensure("header 3",should_retry); - ensure_approximately_equals("header 3", seconds_to_wait, 0.0F, 6); + ensure("header 4",should_retry); + ensure_approximately_equals("header 4", seconds_to_wait, 0.0F, 6); response->release(); } + + // Timeout should be clamped at max. + policy.onFailure(500,LLSD()); + should_retry = policy.shouldRetry(seconds_to_wait); + ensure("header 5", should_retry); + ensure_approximately_equals("header 5", seconds_to_wait, 644.0F, 6); + + // No more retries. + policy.onFailure(500,LLSD()); + should_retry = policy.shouldRetry(seconds_to_wait); + ensure("header 6", !should_retry); } } -- cgit v1.3 From e3bad9fb86c3f44ad67488402ce2e46743a2422e Mon Sep 17 00:00:00 2001 From: "Brad Payne (Vir Linden)" Date: Tue, 16 Apr 2013 20:27:49 -0400 Subject: SH-4061 WIP - fix for build issues on mac, reset the retry policy on success. --- indra/newview/llhttpretrypolicy.cpp | 18 ++++++++++--- indra/newview/llhttpretrypolicy.h | 16 +++++++++--- indra/newview/lltexturefetch.cpp | 4 +++ indra/newview/tests/llhttpretrypolicy_test.cpp | 35 +++++++++++++++++++++----- indra/test/lltut.h | 10 ++++++++ 5 files changed, 69 insertions(+), 14 deletions(-) mode change 100644 => 100755 indra/test/lltut.h (limited to 'indra/newview/llhttpretrypolicy.h') diff --git a/indra/newview/llhttpretrypolicy.cpp b/indra/newview/llhttpretrypolicy.cpp index 10b923be5a..80d97e4362 100755 --- a/indra/newview/llhttpretrypolicy.cpp +++ b/indra/newview/llhttpretrypolicy.cpp @@ -32,11 +32,16 @@ LLAdaptiveRetryPolicy::LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 b mMinDelay(min_delay), mMaxDelay(max_delay), mBackoffFactor(backoff_factor), - mMaxRetries(max_retries), - mDelay(min_delay), - mRetryCount(0), - mShouldRetry(true) + mMaxRetries(max_retries) { + init(); +} + +void LLAdaptiveRetryPolicy::init() +{ + mDelay = mMinDelay; + mRetryCount = 0; + mShouldRetry = true; } bool LLAdaptiveRetryPolicy::getRetryAfter(const LLSD& headers, F32& retry_header_time) @@ -59,6 +64,11 @@ bool LLAdaptiveRetryPolicy::getRetryAfter(const LLCore::HttpHeaders *headers, F3 return false; } +void LLAdaptiveRetryPolicy::onSuccess() +{ + init(); +} + void LLAdaptiveRetryPolicy::onFailure(S32 status, const LLSD& headers) { F32 retry_header_time; diff --git a/indra/newview/llhttpretrypolicy.h b/indra/newview/llhttpretrypolicy.h index 6f63f047de..1fb0cac03f 100755 --- a/indra/newview/llhttpretrypolicy.h +++ b/indra/newview/llhttpretrypolicy.h @@ -42,7 +42,11 @@ class LLHTTPRetryPolicy: public LLThreadSafeRefCount { public: LLHTTPRetryPolicy() {} + virtual ~LLHTTPRetryPolicy() {} + // Call after a sucess to reset retry state. + + virtual void onSuccess() = 0; // Call once after an HTTP failure to update state. virtual void onFailure(S32 status, const LLSD& headers) = 0; @@ -58,6 +62,9 @@ class LLAdaptiveRetryPolicy: public LLHTTPRetryPolicy public: LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries); + // virtual + void onSuccess(); + // virtual void onFailure(S32 status, const LLSD& headers); // virtual @@ -66,16 +73,17 @@ public: bool shouldRetry(F32& seconds_to_wait) const; protected: + void init(); bool getRetryAfter(const LLSD& headers, F32& retry_header_time); bool getRetryAfter(const LLCore::HttpHeaders *headers, F32& retry_header_time); void onFailureCommon(S32 status, bool has_retry_header_time, F32 retry_header_time); private: - F32 mMinDelay; // delay never less than this value - F32 mMaxDelay; // delay never exceeds this value - F32 mBackoffFactor; // delay increases by this factor after each retry, up to mMaxDelay. - U32 mMaxRetries; // maximum number of times shouldRetry will return true. + const F32 mMinDelay; // delay never less than this value + const F32 mMaxDelay; // delay never exceeds this value + const F32 mBackoffFactor; // delay increases by this factor after each retry, up to mMaxDelay. + const U32 mMaxRetries; // maximum number of times shouldRetry will return true. F32 mDelay; // current default delay. U32 mRetryCount; // number of times shouldRetry has been called. LLTimer mRetryTimer; // time until next retry. diff --git a/indra/newview/lltexturefetch.cpp b/indra/newview/lltexturefetch.cpp index 1a6a308230..774c925fa1 100755 --- a/indra/newview/lltexturefetch.cpp +++ b/indra/newview/lltexturefetch.cpp @@ -1976,6 +1976,10 @@ void LLTextureFetchWorker::onCompleted(LLCore::HttpHandle handle, LLCore::HttpRe llinfos << mID << " will not retry" << llendl; } } + else + { + mFetchRetryPolicy.onSuccess(); + } LL_DEBUGS("Texture") << "HTTP COMPLETE: " << mID << " status: " << status.toHex() diff --git a/indra/newview/tests/llhttpretrypolicy_test.cpp b/indra/newview/tests/llhttpretrypolicy_test.cpp index 6fa3a57364..ed7f3ba326 100755 --- a/indra/newview/tests/llhttpretrypolicy_test.cpp +++ b/indra/newview/tests/llhttpretrypolicy_test.cpp @@ -45,7 +45,7 @@ void RetryPolicyTestObject::test<1>() LLSD headers; F32 wait_seconds; - // No retry until we've finished a try. + // No retry until we've failed a try. ensure("never retry 0", !never_retry.shouldRetry(wait_seconds)); // 0 retries max. @@ -74,7 +74,7 @@ void RetryPolicyTestObject::test<3>() bool should_retry; U32 frac_bits = 6; - // No retry until we've finished a try. + // No retry until we've failed a try. ensure("basic_retry 0", !basic_retry.shouldRetry(wait_seconds)); // Starting wait 1.0 @@ -105,6 +105,29 @@ void RetryPolicyTestObject::test<3>() basic_retry.onFailure(500,headers); should_retry = basic_retry.shouldRetry(wait_seconds); ensure("basic_retry 5", !should_retry); + + // Max retries, should fail now. + basic_retry.onFailure(500,headers); + should_retry = basic_retry.shouldRetry(wait_seconds); + ensure("basic_retry 5", !should_retry); + + // After a success, should reset to the starting state. + basic_retry.onSuccess(); + + // No retry until we've failed a try. + ensure("basic_retry 6", !basic_retry.shouldRetry(wait_seconds)); + + // Starting wait 1.0 + basic_retry.onFailure(500,headers); + should_retry = basic_retry.shouldRetry(wait_seconds); + ensure("basic_retry 7", should_retry); + ensure_approximately_equals("basic_retry 7", wait_seconds, 1.0F, frac_bits); + + // Double wait to 2.0 + basic_retry.onFailure(500,headers); + should_retry = basic_retry.shouldRetry(wait_seconds); + ensure("basic_retry 8", should_retry); + ensure_approximately_equals("basic_retry 8", wait_seconds, 2.0F, frac_bits); } // Retries should stop as soon as a non-5xx error is received. @@ -221,7 +244,7 @@ void RetryPolicyTestObject::test<6>() success = getSecondsUntilRetryAfter(str3, seconds_to_wait); std::cerr << " str3 [" << str3 << "]" << std::endl; ensure("parse 3", success); - ensure_approximately_equals("parse 3", seconds_to_wait, 44.0F, 2); + ensure_approximately_equals_range("parse 3", seconds_to_wait, 44.0F, 2.0F); } // Test retry-after field in both llmessage and CoreHttp headers. @@ -237,7 +260,7 @@ void RetryPolicyTestObject::test<7>() F32 seconds_to_wait; bool should_retry; - // No retry until we've finished a try. + // No retry until we've failed a try. ensure("header 0", !policy.shouldRetry(seconds_to_wait)); // no retry header, use default. @@ -252,7 +275,7 @@ void RetryPolicyTestObject::test<7>() policy.onFailure(503,sd_headers); should_retry = policy.shouldRetry(seconds_to_wait); ensure("header 2", should_retry); - ensure_approximately_equals("header 2", seconds_to_wait, 7.0F, 2); + ensure_approximately_equals_range("header 2", seconds_to_wait, 7.0F, 2.0F); LLCore::HttpResponse *response; LLCore::HttpHeaders *headers; @@ -279,7 +302,7 @@ void RetryPolicyTestObject::test<7>() policy.onFailure(response); should_retry = policy.shouldRetry(seconds_to_wait); ensure("header 4",should_retry); - ensure_approximately_equals("header 4", seconds_to_wait, 77.0F, 2); + ensure_approximately_equals_range("header 4", seconds_to_wait, 77.0F, 2.0F); response->release(); // Timeout should be clamped at max. diff --git a/indra/test/lltut.h b/indra/test/lltut.h old mode 100644 new mode 100755 index 55d84bcaca..243e869be7 --- a/indra/test/lltut.h +++ b/indra/test/lltut.h @@ -65,6 +65,16 @@ namespace tut ensure_approximately_equals(NULL, actual, expected, frac_bits); } + inline void ensure_approximately_equals_range(const char *msg, F32 actual, F32 expected, F32 delta) + { + if (fabs(actual-expected)>delta) + { + std::stringstream ss; + ss << (msg?msg:"") << (msg?": ":"") << "not equal actual: " << actual << " expected: " << expected << " tolerance: " << delta; + throw tut::failure(ss.str().c_str()); + } + } + inline void ensure_memory_matches(const char* msg,const void* actual, U32 actual_len, const void* expected,U32 expected_len) { if((expected_len != actual_len) || -- cgit v1.3 From 96a2173c643e145a6f5d4964282c4d43f0fc0c3e Mon Sep 17 00:00:00 2001 From: "Brad Payne (Vir Linden)" Date: Fri, 10 May 2013 09:32:30 -0400 Subject: SH-4176 WIP - allow retries on 4xx errors if enabled by flag. So enable in the case of appearance requests. --- indra/newview/llappearancemgr.cpp | 3 ++- indra/newview/llhttpretrypolicy.cpp | 7 ++++--- indra/newview/llhttpretrypolicy.h | 3 ++- indra/newview/tests/llhttpretrypolicy_test.cpp | 15 +++++++++++---- 4 files changed, 19 insertions(+), 9 deletions(-) (limited to 'indra/newview/llhttpretrypolicy.h') diff --git a/indra/newview/llappearancemgr.cpp b/indra/newview/llappearancemgr.cpp index 3c141aa37a..84a494186f 100755 --- a/indra/newview/llappearancemgr.cpp +++ b/indra/newview/llappearancemgr.cpp @@ -2871,7 +2871,8 @@ class RequestAgentUpdateAppearanceResponder: public LLHTTPClient::Responder public: RequestAgentUpdateAppearanceResponder() { - mRetryPolicy = new LLAdaptiveRetryPolicy(1.0, 32.0, 2.0, 10); + bool retry_on_4xx = true; + mRetryPolicy = new LLAdaptiveRetryPolicy(1.0, 32.0, 2.0, 10, retry_on_4xx); } virtual ~RequestAgentUpdateAppearanceResponder() diff --git a/indra/newview/llhttpretrypolicy.cpp b/indra/newview/llhttpretrypolicy.cpp index 80d97e4362..1512b46103 100755 --- a/indra/newview/llhttpretrypolicy.cpp +++ b/indra/newview/llhttpretrypolicy.cpp @@ -28,11 +28,12 @@ #include "llhttpretrypolicy.h" -LLAdaptiveRetryPolicy::LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries): +LLAdaptiveRetryPolicy::LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries, bool retry_on_4xx): mMinDelay(min_delay), mMaxDelay(max_delay), mBackoffFactor(backoff_factor), - mMaxRetries(max_retries) + mMaxRetries(max_retries), + mRetryOn4xx(retry_on_4xx) { init(); } @@ -108,7 +109,7 @@ void LLAdaptiveRetryPolicy::onFailureCommon(S32 status, bool has_retry_header_ti llinfos << "Too many retries " << mRetryCount << ", will not retry" << llendl; mShouldRetry = false; } - if (!isHttpServerErrorStatus(status)) + if (!mRetryOn4xx && !isHttpServerErrorStatus(status)) { llinfos << "Non-server error " << status << ", will not retry" << llendl; mShouldRetry = false; diff --git a/indra/newview/llhttpretrypolicy.h b/indra/newview/llhttpretrypolicy.h index 1fb0cac03f..5b1a1d79e0 100755 --- a/indra/newview/llhttpretrypolicy.h +++ b/indra/newview/llhttpretrypolicy.h @@ -60,7 +60,7 @@ public: class LLAdaptiveRetryPolicy: public LLHTTPRetryPolicy { public: - LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries); + LLAdaptiveRetryPolicy(F32 min_delay, F32 max_delay, F32 backoff_factor, U32 max_retries, bool retry_on_4xx = false); // virtual void onSuccess(); @@ -88,6 +88,7 @@ private: U32 mRetryCount; // number of times shouldRetry has been called. LLTimer mRetryTimer; // time until next retry. bool mShouldRetry; // Becomes false after too many retries, or the wrong sort of status received, etc. + bool mRetryOn4xx; // Normally only retry on 5xx server errors. }; #endif diff --git a/indra/newview/tests/llhttpretrypolicy_test.cpp b/indra/newview/tests/llhttpretrypolicy_test.cpp index ed7f3ba326..25e6de46d9 100755 --- a/indra/newview/tests/llhttpretrypolicy_test.cpp +++ b/indra/newview/tests/llhttpretrypolicy_test.cpp @@ -56,12 +56,19 @@ void RetryPolicyTestObject::test<1>() template<> template<> void RetryPolicyTestObject::test<2>() { - LLAdaptiveRetryPolicy retry404(1.0,2.0,3.0,10); LLSD headers; F32 wait_seconds; - - retry404.onFailure(404,headers); - ensure("no retry on 404", !retry404.shouldRetry(wait_seconds)); + + // Normally only retry on server error (5xx) + LLAdaptiveRetryPolicy noRetry404(1.0,2.0,3.0,10); + noRetry404.onFailure(404,headers); + ensure("no retry on 404", !noRetry404.shouldRetry(wait_seconds)); + + // Can retry on 4xx errors if enabled by flag. + bool do_retry_4xx = true; + LLAdaptiveRetryPolicy doRetry404(1.0,2.0,3.0,10,do_retry_4xx); + doRetry404.onFailure(404,headers); + ensure("do retry on 404", doRetry404.shouldRetry(wait_seconds)); } template<> template<> -- cgit v1.3 From 8f4f7452308d41467b021ae0da821b33f559dd79 Mon Sep 17 00:00:00 2001 From: "Brad Payne (Vir Linden)" Date: Wed, 3 Jul 2013 12:45:56 -0400 Subject: SH-4226 WIP - try to be smarter about when to send appearance update requests, removed many redundant calls --- indra/newview/llappearancemgr.cpp | 413 +++++++++++++++++++++--------------- indra/newview/llappearancemgr.h | 5 +- indra/newview/llhttpretrypolicy.cpp | 5 + indra/newview/llhttpretrypolicy.h | 4 + indra/newview/llvoavatar.cpp | 15 +- 5 files changed, 270 insertions(+), 172 deletions(-) (limited to 'indra/newview/llhttpretrypolicy.h') diff --git a/indra/newview/llappearancemgr.cpp b/indra/newview/llappearancemgr.cpp index 12c8c82af4..485c47b2f7 100755 --- a/indra/newview/llappearancemgr.cpp +++ b/indra/newview/llappearancemgr.cpp @@ -3037,158 +3037,274 @@ void LLAppearanceMgr::updateClothingOrderingInfo(LLUUID cat_id, class RequestAgentUpdateAppearanceResponder: public LLHTTPClient::Responder { LOG_CLASS(RequestAgentUpdateAppearanceResponder); + + friend class LLAppearanceMgr; + public: - RequestAgentUpdateAppearanceResponder() - { - bool retry_on_4xx = true; - mRetryPolicy = new LLAdaptiveRetryPolicy(1.0, 32.0, 2.0, 10, retry_on_4xx); - } + RequestAgentUpdateAppearanceResponder(); - virtual ~RequestAgentUpdateAppearanceResponder() - { - } + virtual ~RequestAgentUpdateAppearanceResponder(); + +private: + // Called when sendServerAppearanceUpdate called. May or may not + // trigger a request depending on various bits of state. + void onRequestRequested(); + + // Post the actual appearance request to cap. + void sendRequest(); + + void debugCOF(const LLSD& content); protected: // Successful completion. - /* virtual */ void httpSuccess() - { - const LLSD& content = getContent(); - if (!content.isMap()) - { - failureResult(HTTP_INTERNAL_ERROR, "Malformed response contents", content); - return; - } - if (content["success"].asBoolean()) - { - //LL_DEBUGS("Avatar") << dumpResponse() << LL_ENDL; - if (gSavedSettings.getBOOL("DebugAvatarAppearanceMessage")) - { - dump_sequential_xml(gAgentAvatarp->getFullname() + "_appearance_request_ok", content); - } - } - else - { - failureResult(HTTP_INTERNAL_ERROR, "Non-success response", content); - } - } + /* virtual */ void httpSuccess(); // Error - /*virtual*/ void httpFailure() + /*virtual*/ void httpFailure(); + + void onFailure(); + void onSuccess(); + + S32 mInFlightCounter; + LLTimer mInFlightTimer; + LLPointer mRetryPolicy; +}; + +RequestAgentUpdateAppearanceResponder::RequestAgentUpdateAppearanceResponder() +{ + bool retry_on_4xx = true; + mRetryPolicy = new LLAdaptiveRetryPolicy(1.0, 32.0, 2.0, 10, retry_on_4xx); + mInFlightCounter = 0; +} + +RequestAgentUpdateAppearanceResponder::~RequestAgentUpdateAppearanceResponder() +{ +} + +void RequestAgentUpdateAppearanceResponder::onRequestRequested() +{ + // If we have already received an update for this or higher cof version, ignore. + S32 cof_version = LLAppearanceMgr::instance().getCOFVersion(); + S32 last_rcv = gAgentAvatarp->mLastUpdateReceivedCOFVersion; + S32 last_req = gAgentAvatarp->mLastUpdateRequestCOFVersion; + LL_DEBUGS("Avatar") << "cof_version " << cof_version + << " last_rcv " << last_rcv + << " last_req " << last_req + << " in flight " << mInFlightCounter << llendl; + if ((mInFlightCounter>0) && (mInFlightTimer.hasExpired())) + { + LL_WARNS("Avatar") << "in flight timer expired, resetting " << llendl; + mInFlightCounter = 0; + } + if (cof_version < last_rcv) { - LL_WARNS("Avatar") << "appearance update request failed, status " - << getStatus() << " reason " << getReason() << LL_ENDL; + LL_DEBUGS("Avatar") << "Have already received update for cof version " << last_rcv + << " will not request for " << cof_version << llendl; + return; + } + if (mInFlightCounter>0 && last_req >= cof_version) + { + LL_DEBUGS("Avatar") << "Request already in flight for cof version " << last_req + << " will not request for " << cof_version << llendl; + return; + } - if (gSavedSettings.getBOOL("DebugAvatarAppearanceMessage")) - { - const LLSD& content = getContent(); - dump_sequential_xml(gAgentAvatarp->getFullname() + "_appearance_request_error", content); - debugCOF(content); - } - onFailure(); + // Actually send the request. + LL_DEBUGS("Avatar") << "Will send request for cof_version " << cof_version << llendl; + mRetryPolicy->reset(); + sendRequest(); +} + +void RequestAgentUpdateAppearanceResponder::sendRequest() +{ + if (gAgentAvatarp->isEditingAppearance()) + { + // don't send out appearance updates if in appearance editing mode + return; } - void onFailure() + if (!gAgent.getRegion()) { - F32 seconds_to_wait; - mRetryPolicy->onFailure(getStatus(), getResponseHeaders()); - if (mRetryPolicy->shouldRetry(seconds_to_wait)) - { - llinfos << "retrying" << llendl; - doAfterInterval(boost::bind(&LLAppearanceMgr::requestServerAppearanceUpdate, - LLAppearanceMgr::getInstance(), - LLHTTPClient::ResponderPtr(this)), - seconds_to_wait); - } - else + llwarns << "Region not set, cannot request server appearance update" << llendl; + return; + } + if (gAgent.getRegion()->getCentralBakeVersion()==0) + { + llwarns << "Region does not support baking" << llendl; + } + std::string url = gAgent.getRegion()->getCapability("UpdateAvatarAppearance"); + if (url.empty()) + { + llwarns << "No cap for UpdateAvatarAppearance." << llendl; + return; + } + + LLSD body; + S32 cof_version = LLAppearanceMgr::instance().getCOFVersion(); + if (gSavedSettings.getBOOL("DebugAvatarExperimentalServerAppearanceUpdate")) + { + body = LLAppearanceMgr::instance().dumpCOF(); + } + else + { + body["cof_version"] = cof_version; + if (gSavedSettings.getBOOL("DebugForceAppearanceRequestFailure")) { - llwarns << "giving up after too many retries" << llendl; + body["cof_version"] = cof_version+999; } - } + } + LL_DEBUGS("Avatar") << "request url " << url << " my_cof_version " << cof_version << llendl; + + mInFlightCounter++; + mInFlightTimer.setTimerExpirySec(30.0); + LLHTTPClient::post(url, body, this); + llassert(cof_version >= gAgentAvatarp->mLastUpdateRequestCOFVersion); + gAgentAvatarp->mLastUpdateRequestCOFVersion = cof_version; +} - void debugCOF(const LLSD& content) +void RequestAgentUpdateAppearanceResponder::debugCOF(const LLSD& content) +{ + LL_INFOS("Avatar") << "AIS COF, version received: " << content["expected"].asInteger() + << " ================================= " << llendl; + std::set ais_items, local_items; + const LLSD& cof_raw = content["cof_raw"]; + for (LLSD::array_const_iterator it = cof_raw.beginArray(); + it != cof_raw.endArray(); ++it) { - LL_INFOS("Avatar") << "AIS COF, version received: " << content["expected"].asInteger() - << " ================================= " << llendl; - std::set ais_items, local_items; - const LLSD& cof_raw = content["cof_raw"]; - for (LLSD::array_const_iterator it = cof_raw.beginArray(); - it != cof_raw.endArray(); ++it) + const LLSD& item = *it; + if (item["parent_id"].asUUID() == LLAppearanceMgr::instance().getCOF()) { - const LLSD& item = *it; - if (item["parent_id"].asUUID() == LLAppearanceMgr::instance().getCOF()) + ais_items.insert(item["item_id"].asUUID()); + if (item["type"].asInteger() == 24) // link { - ais_items.insert(item["item_id"].asUUID()); - if (item["type"].asInteger() == 24) // link - { - LL_INFOS("Avatar") << "AIS Link: item_id: " << item["item_id"].asUUID() - << " linked_item_id: " << item["asset_id"].asUUID() - << " name: " << item["name"].asString() - << llendl; - } - else if (item["type"].asInteger() == 25) // folder link - { - LL_INFOS("Avatar") << "AIS Folder link: item_id: " << item["item_id"].asUUID() - << " linked_item_id: " << item["asset_id"].asUUID() - << " name: " << item["name"].asString() - << llendl; + LL_INFOS("Avatar") << "AIS Link: item_id: " << item["item_id"].asUUID() + << " linked_item_id: " << item["asset_id"].asUUID() + << " name: " << item["name"].asString() + << llendl; + } + else if (item["type"].asInteger() == 25) // folder link + { + LL_INFOS("Avatar") << "AIS Folder link: item_id: " << item["item_id"].asUUID() + << " linked_item_id: " << item["asset_id"].asUUID() + << " name: " << item["name"].asString() + << llendl; - } - else - { - LL_INFOS("Avatar") << "AIS Other: item_id: " << item["item_id"].asUUID() - << " linked_item_id: " << item["asset_id"].asUUID() - << " name: " << item["name"].asString() - << " type: " << item["type"].asInteger() - << llendl; - } } - } - LL_INFOS("Avatar") << llendl; - LL_INFOS("Avatar") << "Local COF, version requested: " << content["observed"].asInteger() - << " ================================= " << llendl; - LLInventoryModel::cat_array_t cat_array; - LLInventoryModel::item_array_t item_array; - gInventory.collectDescendents(LLAppearanceMgr::instance().getCOF(), - cat_array,item_array,LLInventoryModel::EXCLUDE_TRASH); - for (S32 i=0; igetUUID()); - LL_INFOS("Avatar") << "LOCAL: item_id: " << inv_item->getUUID() - << " linked_item_id: " << inv_item->getLinkedUUID() - << " name: " << inv_item->getName() - << " parent: " << inv_item->getParentUUID() - << llendl; - } - LL_INFOS("Avatar") << " ================================= " << llendl; - S32 local_only = 0, ais_only = 0; - for (std::set::iterator it = local_items.begin(); it != local_items.end(); ++it) - { - if (ais_items.find(*it) == ais_items.end()) + else { - LL_INFOS("Avatar") << "LOCAL ONLY: " << *it << llendl; - local_only++; + LL_INFOS("Avatar") << "AIS Other: item_id: " << item["item_id"].asUUID() + << " linked_item_id: " << item["asset_id"].asUUID() + << " name: " << item["name"].asString() + << " type: " << item["type"].asInteger() + << llendl; } } - for (std::set::iterator it = ais_items.begin(); it != ais_items.end(); ++it) + } + LL_INFOS("Avatar") << llendl; + LL_INFOS("Avatar") << "Local COF, version requested: " << content["observed"].asInteger() + << " ================================= " << llendl; + LLInventoryModel::cat_array_t cat_array; + LLInventoryModel::item_array_t item_array; + gInventory.collectDescendents(LLAppearanceMgr::instance().getCOF(), + cat_array,item_array,LLInventoryModel::EXCLUDE_TRASH); + for (S32 i=0; igetUUID()); + LL_INFOS("Avatar") << "LOCAL: item_id: " << inv_item->getUUID() + << " linked_item_id: " << inv_item->getLinkedUUID() + << " name: " << inv_item->getName() + << " parent: " << inv_item->getParentUUID() + << llendl; + } + LL_INFOS("Avatar") << " ================================= " << llendl; + S32 local_only = 0, ais_only = 0; + for (std::set::iterator it = local_items.begin(); it != local_items.end(); ++it) + { + if (ais_items.find(*it) == ais_items.end()) { - if (local_items.find(*it) == local_items.end()) - { - LL_INFOS("Avatar") << "AIS ONLY: " << *it << llendl; - ais_only++; - } + LL_INFOS("Avatar") << "LOCAL ONLY: " << *it << llendl; + local_only++; } - if (local_only==0 && ais_only==0) + } + for (std::set::iterator it = ais_items.begin(); it != ais_items.end(); ++it) + { + if (local_items.find(*it) == local_items.end()) { - LL_INFOS("Avatar") << "COF contents identical, only version numbers differ (req " - << content["observed"].asInteger() - << " rcv " << content["expected"].asInteger() - << ")" << llendl; + LL_INFOS("Avatar") << "AIS ONLY: " << *it << llendl; + ais_only++; } } + if (local_only==0 && ais_only==0) + { + LL_INFOS("Avatar") << "COF contents identical, only version numbers differ (req " + << content["observed"].asInteger() + << " rcv " << content["expected"].asInteger() + << ")" << llendl; + } +} + +/* virtual */ void RequestAgentUpdateAppearanceResponder::httpSuccess() +{ + const LLSD& content = getContent(); + if (!content.isMap()) + { + failureResult(HTTP_INTERNAL_ERROR, "Malformed response contents", content); + return; + } + if (content["success"].asBoolean()) + { + //LL_DEBUGS("Avatar") << dumpResponse() << LL_ENDL; + if (gSavedSettings.getBOOL("DebugAvatarAppearanceMessage")) + { + dump_sequential_xml(gAgentAvatarp->getFullname() + "_appearance_request_ok", content); + } + + onSuccess(); + } + else + { + failureResult(HTTP_INTERNAL_ERROR, "Non-success response", content); + } +} + +void RequestAgentUpdateAppearanceResponder::onSuccess() +{ + mInFlightCounter = llmax(mInFlightCounter-1,0); +} + +/*virtual*/ void RequestAgentUpdateAppearanceResponder::httpFailure() +{ + LL_WARNS("Avatar") << "appearance update request failed, status " + << getStatus() << " reason " << getReason() << LL_ENDL; + + if (gSavedSettings.getBOOL("DebugAvatarAppearanceMessage")) + { + const LLSD& content = getContent(); + dump_sequential_xml(gAgentAvatarp->getFullname() + "_appearance_request_error", content); + debugCOF(content); + } + onFailure(); +} + +void RequestAgentUpdateAppearanceResponder::onFailure() +{ + mInFlightCounter = llmax(mInFlightCounter-1,0); + + F32 seconds_to_wait; + mRetryPolicy->onFailure(getStatus(), getResponseHeaders()); + if (mRetryPolicy->shouldRetry(seconds_to_wait)) + { + llinfos << "retrying" << llendl; + doAfterInterval(boost::bind(&RequestAgentUpdateAppearanceResponder::sendRequest,this), + seconds_to_wait); + } + else + { + llwarns << "giving up after too many retries" << llendl; + } +} - LLPointer mRetryPolicy; -}; LLSD LLAppearanceMgr::dumpCOF() const { @@ -3253,53 +3369,9 @@ LLSD LLAppearanceMgr::dumpCOF() const return result; } -void LLAppearanceMgr::requestServerAppearanceUpdate(LLCurl::ResponderPtr responder_ptr) +void LLAppearanceMgr::requestServerAppearanceUpdate() { - if (gAgentAvatarp->isEditingAppearance()) - { - // don't send out appearance updates if in appearance editing mode - return; - } - - if (!gAgent.getRegion()) - { - llwarns << "Region not set, cannot request server appearance update" << llendl; - return; - } - if (gAgent.getRegion()->getCentralBakeVersion()==0) - { - llwarns << "Region does not support baking" << llendl; - } - std::string url = gAgent.getRegion()->getCapability("UpdateAvatarAppearance"); - if (url.empty()) - { - llwarns << "No cap for UpdateAvatarAppearance." << llendl; - return; - } - - LLSD body; - S32 cof_version = getCOFVersion(); - if (gSavedSettings.getBOOL("DebugAvatarExperimentalServerAppearanceUpdate")) - { - body = dumpCOF(); - } - else - { - body["cof_version"] = cof_version; - if (gSavedSettings.getBOOL("DebugForceAppearanceRequestFailure")) - { - body["cof_version"] = cof_version+999; - } - } - LL_DEBUGS("Avatar") << "request url " << url << " my_cof_version " << cof_version << llendl; - - if (!responder_ptr.get()) - { - responder_ptr = new RequestAgentUpdateAppearanceResponder; - } - LLHTTPClient::post(url, body, responder_ptr); - llassert(cof_version >= gAgentAvatarp->mLastUpdateRequestCOFVersion); - gAgentAvatarp->mLastUpdateRequestCOFVersion = cof_version; + mAppearanceResponder->onRequestRequested(); } class LLIncrementCofVersionResponder : public LLHTTPClient::Responder @@ -3635,7 +3707,8 @@ LLAppearanceMgr::LLAppearanceMgr(): mAttachmentInvLinkEnabled(false), mOutfitIsDirty(false), mOutfitLocked(false), - mIsInUpdateAppearanceFromCOF(false) + mIsInUpdateAppearanceFromCOF(false), + mAppearanceResponder(new RequestAgentUpdateAppearanceResponder) { LLOutfitObserver& outfit_observer = LLOutfitObserver::instance(); diff --git a/indra/newview/llappearancemgr.h b/indra/newview/llappearancemgr.h index b2917cced4..13a2a62717 100755 --- a/indra/newview/llappearancemgr.h +++ b/indra/newview/llappearancemgr.h @@ -38,6 +38,7 @@ class LLWearableHoldingPattern; class LLInventoryCallback; class LLOutfitUnLockTimer; +class RequestAgentUpdateAppearanceResponder; class LLAppearanceMgr: public LLSingleton { @@ -214,7 +215,7 @@ public: bool isInUpdateAppearanceFromCOF() { return mIsInUpdateAppearanceFromCOF; } - void requestServerAppearanceUpdate(LLCurl::ResponderPtr responder_ptr = NULL); + void requestServerAppearanceUpdate(); void incrementCofVersion(LLHTTPClient::ResponderPtr responder_ptr = NULL); @@ -255,6 +256,8 @@ private: bool mOutfitIsDirty; bool mIsInUpdateAppearanceFromCOF; // to detect recursive calls. + LLPointer mAppearanceResponder; + /** * Lock for blocking operations on outfit until server reply or timeout exceed * to avoid unsynchronized outfit state or performing duplicate operations. diff --git a/indra/newview/llhttpretrypolicy.cpp b/indra/newview/llhttpretrypolicy.cpp index 1512b46103..ae429f11f8 100755 --- a/indra/newview/llhttpretrypolicy.cpp +++ b/indra/newview/llhttpretrypolicy.cpp @@ -45,6 +45,11 @@ void LLAdaptiveRetryPolicy::init() mShouldRetry = true; } +void LLAdaptiveRetryPolicy::reset() +{ + init(); +} + bool LLAdaptiveRetryPolicy::getRetryAfter(const LLSD& headers, F32& retry_header_time) { return (headers.has(HTTP_IN_HEADER_RETRY_AFTER) diff --git a/indra/newview/llhttpretrypolicy.h b/indra/newview/llhttpretrypolicy.h index 5b1a1d79e0..cf79e0b401 100755 --- a/indra/newview/llhttpretrypolicy.h +++ b/indra/newview/llhttpretrypolicy.h @@ -53,6 +53,8 @@ public: virtual void onFailure(const LLCore::HttpResponse *response) = 0; virtual bool shouldRetry(F32& seconds_to_wait) const = 0; + + virtual void reset() = 0; }; // Very general policy with geometric back-off after failures, @@ -64,6 +66,8 @@ public: // virtual void onSuccess(); + + void reset(); // virtual void onFailure(S32 status, const LLSD& headers); diff --git a/indra/newview/llvoavatar.cpp b/indra/newview/llvoavatar.cpp index 2fd26eae80..74fb51b943 100755 --- a/indra/newview/llvoavatar.cpp +++ b/indra/newview/llvoavatar.cpp @@ -7014,7 +7014,15 @@ void LLVOAvatar::processAvatarAppearance( LLMessageSystem* mesgsys ) return; } - mLastUpdateReceivedCOFVersion = this_update_cof_version; + // No backsies zone - if we get here, the message should be valid and usable. + if (appearance_version > 0) + { + // Note: + // RequestAgentUpdateAppearanceResponder::onRequestRequested() + // assumes that cof version is only updated with server-bake + // appearance messages. + mLastUpdateReceivedCOFVersion = this_update_cof_version; + } setIsUsingServerBakes(appearance_version > 0); @@ -7061,6 +7069,7 @@ void LLVOAvatar::processAvatarAppearance( LLMessageSystem* mesgsys ) LL_DEBUGS("Avatar") << avString() << " handle visual params, num_params " << num_params << LL_ENDL; BOOL params_changed = FALSE; BOOL interp_params = FALSE; + S32 params_changed_count = 0; for( S32 i = 0; i < num_params; i++ ) { @@ -7070,12 +7079,15 @@ void LLVOAvatar::processAvatarAppearance( LLMessageSystem* mesgsys ) if (is_first_appearance_message || (param->getWeight() != newWeight)) { params_changed = TRUE; + params_changed_count++; if(is_first_appearance_message) { + //LL_DEBUGS("Avatar") << "param slam " << i << " " << newWeight << llendl; param->setWeight(newWeight, FALSE); } else { + //LL_DEBUGS("Avatar") << std::setprecision(5) << " param target " << i << " " << param->getWeight() << " -> " << newWeight << llendl; interp_params = TRUE; param->setAnimationTarget(newWeight, FALSE); } @@ -7087,6 +7099,7 @@ void LLVOAvatar::processAvatarAppearance( LLMessageSystem* mesgsys ) LL_DEBUGS("Avatar") << "Number of params in AvatarAppearance msg (" << num_params << ") does not match number of tweakable params in avatar xml file (" << expected_tweakable_count << "). Processing what we can. object: " << getID() << llendl; } + LL_DEBUGS("Avatar") << "Changed " << params_changed_count << " params" << llendl; if (params_changed) { if (interp_params) -- cgit v1.3