From 741a160913334faa9d2aa5ff853ea4c1a2d08132 Mon Sep 17 00:00:00 2001 From: Aleric Inglewood Date: Fri, 11 Jan 2013 01:28:46 +0100 Subject: [PATCH] Fine tune of private (libcwd) assertion. It turns out that it's possible to receive data before an upload finished when the server sends HTTP_BAD_REQUEST in the middle of the upload (this happens to me when I try to upload a 6000x6000 image to my profile feed, with is a 44MB file). So, in that case the finished upload detection did not fail and we shouldn't assert. --- indra/llmessage/aicurl.cpp | 7 ++++++- indra/llmessage/aicurlprivate.h | 5 ++++- indra/llmessage/aicurlthread.cpp | 32 ++++++++++++++++++++++---------- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/indra/llmessage/aicurl.cpp b/indra/llmessage/aicurl.cpp index cbb5fcc45..2bb76b5ab 100644 --- a/indra/llmessage/aicurl.cpp +++ b/indra/llmessage/aicurl.cpp @@ -1276,7 +1276,7 @@ static int const HTTP_REDIRECTS_DEFAULT = 10; LLChannelDescriptors const BufferedCurlEasyRequest::sChannels; -BufferedCurlEasyRequest::BufferedCurlEasyRequest() : mRequestTransferedBytes(0), mResponseTransferedBytes(0), mBufferEventsTarget(NULL) +BufferedCurlEasyRequest::BufferedCurlEasyRequest() : mRequestTransferedBytes(0), mResponseTransferedBytes(0), mBufferEventsTarget(NULL), mStatus(HTTP_INTERNAL_ERROR) { AICurlInterface::Stats::BufferedCurlEasyRequest_count++; } @@ -1336,8 +1336,13 @@ void BufferedCurlEasyRequest::resetState(void) // Call base class implementation. CurlEasyRequest::resetState(); + // Reset local variables. mOutput.reset(); mInput.reset(); + mRequestTransferedBytes = 0; + mResponseTransferedBytes = 0; + mBufferEventsTarget = NULL; + mStatus = HTTP_INTERNAL_ERROR; } void BufferedCurlEasyRequest::print_diagnostics(CURLcode code) diff --git a/indra/llmessage/aicurlprivate.h b/indra/llmessage/aicurlprivate.h index bd6207e2d..d9944ccb3 100644 --- a/indra/llmessage/aicurlprivate.h +++ b/indra/llmessage/aicurlprivate.h @@ -88,7 +88,7 @@ class HTTPTimeout : public LLRefCount { bool data_sent(size_t n); // Called when data is received. Returns true if transfer timed out. - bool data_received(size_t n); + bool data_received(size_t n/*,*/ ASSERT_ONLY_COMMA(bool upload_error_status = false)); // Called immediately before done() after curl finished, with code. void done(AICurlEasyRequest_wat const& curlEasyRequest_w, CURLcode code); @@ -498,6 +498,9 @@ class BufferedCurlEasyRequest : public CurlEasyRequest { // Return pointer to the ThreadSafe (wrapped) version of this object. ThreadSafeBufferedCurlEasyRequest* get_lockobj(void); ThreadSafeBufferedCurlEasyRequest const* get_lockobj(void) const; + // Return true when an error code was received that can occur before the upload finished. + // So far the only such error I've seen is HTTP_BAD_REQUEST. + bool upload_error_status(void) const { return mStatus == HTTP_BAD_REQUEST /*&& mStatus != HTTP_INTERNAL_ERROR*/; } // Return true when prepRequest was already called and the object has not been // invalidated as a result of calling timed_out(). diff --git a/indra/llmessage/aicurlthread.cpp b/indra/llmessage/aicurlthread.cpp index 6b67733bd..510789676 100644 --- a/indra/llmessage/aicurlthread.cpp +++ b/indra/llmessage/aicurlthread.cpp @@ -1871,7 +1871,13 @@ void HTTPTimeout::upload_finished(void) // queued--><--DNS lookup + connect + send headers-->[<--send body (if any)-->]<--replydelay--><--receive headers + body--><--done // ^ ^ ^ ^ ^ ^ ^ ^ // | | | | | | | | -bool HTTPTimeout::data_received(size_t n) +bool HTTPTimeout::data_received(size_t n/*,*/ +#ifdef CWDEBUG + ASSERT_ONLY_COMMA(bool upload_error_status) +#else + ASSERT_ONLY_COMMA(bool) +#endif + ) { // The HTTP header of the reply is the first thing we receive. if (mNothingReceivedYet && n > 0) @@ -1882,7 +1888,8 @@ bool HTTPTimeout::data_received(size_t n) // because in that case it is impossible to detect the difference between connecting and waiting for a reply without // using CURLOPT_DEBUGFUNCTION. Note that mDebugIsHeadOrGetMethod is only valid when the debug channel 'curlio' is on, // because it is set in the debug callback function. - Debug(llassert(AICurlEasyRequest_wat(*mLockObj)->mDebugIsHeadOrGetMethod || !dc::curlio.is_on())); + // This is also normal if we received a HTTP header with an error status, since that can interrupt our upload. + Debug(llassert(upload_error_status || AICurlEasyRequest_wat(*mLockObj)->mDebugIsHeadOrGetMethod || !dc::curlio.is_on())); // 'Upload finished' detection failed, generate it now. upload_finished(); } @@ -2395,23 +2402,18 @@ size_t BufferedCurlEasyRequest::curlHeaderCallback(char* data, size_t size, size char const* const header_line = static_cast(data); size_t const header_len = size * nmemb; - if (self_w->httptimeout()->data_received(header_len)) // Update timeout administration. - { - // Transfer timed out. Return 0 which will abort with error CURLE_WRITE_ERROR. - return 0; - } if (!header_len) { return header_len; } std::string header(header_line, header_len); + bool done = false; if (!LLStringUtil::_isASCII(header)) { - return header_len; + done = true; } - // Per HTTP spec the first header line must be the status line. - if (header.substr(0, 5) == "HTTP/") + else if (header.substr(0, 5) == "HTTP/") { std::string::iterator const begin = header.begin(); std::string::iterator const end = header.end(); @@ -2445,6 +2447,16 @@ size_t BufferedCurlEasyRequest::curlHeaderCallback(char* data, size_t size, size } self_w->received_HTTP_header(); self_w->setStatusAndReason(status, reason); + done = true; + } + // Update timeout administration. This must be done after the status is already known. + if (self_w->httptimeout()->data_received(header_len/*,*/ ASSERT_ONLY_COMMA(self_w->upload_error_status()))) + { + // Transfer timed out. Return 0 which will abort with error CURLE_WRITE_ERROR. + return 0; + } + if (done) + { return header_len; }