LLCurlRequest time out fixes.

Also some more cleanup on exit improvements.

indra/llmessage/aicurl.cpp

@@ -554,7 +554,7 @@ void CurlEasyHandle::handle_easy_error(CURLcode code)
 }
 
 // Throws AICurlNoEasyHandle.
-CurlEasyHandle::CurlEasyHandle(void) : mActiveMultiHandle(NULL), mErrorBuffer(NULL)
+CurlEasyHandle::CurlEasyHandle(void) : mActiveMultiHandle(NULL), mErrorBuffer(NULL), mQueuedForRemoval(false)
 #ifdef SHOW_ASSERT
 	, mRemovedPerCommand(true)
 #endif
@@ -815,7 +815,7 @@ void CurlEasyRequest::revokeCallbacks(void)
   mWriteCallback = &noWriteCallback;
   mReadCallback = &noReadCallback;
   mSSLCtxCallback = &noSSLCtxCallback;
-  if (active() && !LLApp::isExiting())
+  if (active() && !no_warning())
   {
 	llwarns << "Revoking callbacks on a still active CurlEasyRequest object!" << llendl;
   }

indra/llmessage/aicurlprivate.h

@@ -98,10 +98,17 @@ class CurlEasyHandle : public boost::noncopyable, protected AICurlEasyHandleEven
 	// Pause and unpause a connection.
 	CURLcode pause(int bitmask);
 
+	// Called when a request is queued for removal. In that case a race between the actual removal
+	// and revoking of the callbacks is harmless (and happens for the raw non-statemachine version).
+	void remove_queued(void) { mQueuedForRemoval = true; }
+	// In case it's added after being removed.
+	void add_queued(void) { mQueuedForRemoval = false; }
+
   private:
 	CURL* mEasyHandle;
 	CURLM* mActiveMultiHandle;
 	char* mErrorBuffer;
+	bool mQueuedForRemoval;			// Set if the easy handle is (probably) added to the multi handle, but is queued for removal.
 #ifdef SHOW_ASSERT
   public:
 	bool mRemovedPerCommand;		// Set if mActiveMultiHandle was reset as per command from the main thread.
@@ -121,6 +128,11 @@ class CurlEasyHandle : public boost::noncopyable, protected AICurlEasyHandleEven
 	// Only valid when setErrorBuffer was called and the curl_easy function returned an error.
 	std::string getErrorString(void) const { return mErrorBuffer ? mErrorBuffer : "(null)"; }
 
+	// Returns true when it is expected that the parent will revoke callbacks before the curl
+	// easy handle is removed from the multi handle; that usually happens when an external
+	// error demands termination of the request (ie, an expiration).
+	bool no_warning(void) const { return mQueuedForRemoval || LLApp::isExiting(); }
+
 	// Used for debugging purposes.
 	bool operator==(CURL* easy_handle) const { return mEasyHandle == easy_handle; }
 

indra/llmessage/aicurlthread.cpp

@@ -617,6 +617,7 @@ void AICurlThread::cleanup_wakeup_fds(void)
 void AICurlThread::wakeup_thread(void)
 {
   DoutEntering(dc::curl, "AICurlThread::wakeup_thread");
+  llassert(is_main_thread());
 
 #ifdef WINDOWS
 #error Missing implementation
@@ -1084,6 +1085,9 @@ void stopCurlThread(void)
 	  ms_sleep(10);
 	}
 	Dout(dc::curl, "Curl thread" << (curlThreadIsRunning() ? " not" : "") << " stopped after " << ((100 - count) * 10) << "ms.");
+	// Clear the command queue, for a cleaner cleanup.
+	command_queue_wat command_queue_w(command_queue);
+	command_queue_w->clear();
   }
 }
 
@@ -1136,6 +1140,7 @@ void AICurlEasyRequest::addRequest(void)
 #endif
 	// Add a command to add the new request to the multi session to the command queue.
 	command_queue_w->push_back(Command(*this, cmd_add));
+	AICurlEasyRequest_wat(*get())->add_queued();
   }
   // Something was added to the queue, wake up the thread to get it.
   wakeUpCurlThread();
@@ -1188,6 +1193,8 @@ void AICurlEasyRequest::removeRequest(void)
 #endif
 	// Add a command to remove this request from the multi session to the command queue.
 	command_queue_w->push_back(Command(*this, cmd_remove));
+	// Suppress warning that would otherwise happen if the callbacks are revoked before the curl thread removed the request.
+	AICurlEasyRequest_wat(*get())->remove_queued();
   }
   // Something was added to the queue, wake up the thread to get it.
   wakeUpCurlThread();

indra/llmessage/lliopipe.h

@@ -149,7 +149,7 @@ public:
 		// The connection was lost.
 		STATUS_LOST_CONNECTION = -5,
 
-		// The totoal process time has exceeded the timeout.
+		// The total process time has exceeded the timeout.
 		STATUS_EXPIRED = -6,
 
 		// Keep track of the count of codes here.

indra/llmessage/llpumpio.cpp

@@ -207,14 +207,6 @@ bool LLPumpIO::addChain(chain_t const& chain, F32 timeout)
 	if (it == end) return false;
 
 	LLChainInfo info;
-	for(; it != end; ++it)
-	{
-		if ((*it)->hasExpiration())
-		{
-			info.mHasExpiration = true;
-			break;
-		}
-	}
 	info.setTimeoutSeconds(timeout);
 	info.mData = LLIOPipe::buffer_ptr_t(new LLBufferArray);
 	LLLinkInfo link;
@@ -224,13 +216,14 @@ bool LLPumpIO::addChain(chain_t const& chain, F32 timeout)
 #else
 	lldebugs << "LLPumpIO::addChain() " << chain[0] <<llendl;
 #endif
-	it = chain.begin();
 	for(; it != end; ++it)
 	{
+		info.mHasExpiration = info.mHasExpiration || (*it)->hasExpiration();
 		link.mPipe = (*it);
 		link.mChannels = info.mData->nextChannel();
 		info.mChainLinks.push_back(link);
 	}
+
 #if LL_THREADS_APR
 	LLScopedLock lock(mChainsMutex);
 #endif
@@ -250,11 +243,10 @@ bool LLPumpIO::addChain(
 	// description, we need to have that description matched to a
 	// particular buffer.
 	if(!data) return false;
-	if(links.empty()) return false;
+	links_t::const_iterator link = links.begin();
+	links_t::const_iterator const end = links.end();
+	if (link == end) return false;
 
-#if LL_THREADS_APR
-	LLScopedLock lock(mChainsMutex);
-#endif
 #if LL_DEBUG_PIPE_TYPE_IN_PUMP
 	lldebugs << "LLPumpIO::addChain() " << links[0].mPipe << " '"
 		<< typeid(*(links[0].mPipe)).name() << "'" << llendl;
@@ -266,6 +258,17 @@ bool LLPumpIO::addChain(
 	info.mChainLinks = links;
 	info.mData = data;
 	info.mContext = context;
+	for (; link != end; ++link)
+	{
+		if (link->mPipe->hasExpiration())
+		{
+			info.mHasExpiration = true;
+			break;
+		}
+	}
+#if LL_THREADS_APR
+	LLScopedLock lock(mChainsMutex);
+#endif
 	mPendingChains.push_back(info);
 	return true;
 }
@@ -1114,6 +1117,8 @@ bool LLPumpIO::handleChainError(
 	LLChainInfo& chain,
 	LLIOPipe::EStatus error)
 {
+	DoutEntering(dc::notice, "LLPumpIO::handleChainError(" << (void*)&chain << ", " << LLIOPipe::lookupStatusString(error) << ")");
+
 	LLMemType m1(LLMemType::MTYPE_IO_PUMP);
 	links_t::reverse_iterator rit;
 	if(chain.mHead == chain.mChainLinks.end())

indra/llmessage/llurlrequest.cpp

@@ -283,10 +283,24 @@ LLIOPipe::EStatus LLURLRequest::handleError(
 	LLIOPipe::EStatus status,
 	LLPumpIO* pump)
 {
+	DoutEntering(dc::curl, "LLURLRequest::handleError(" << LLIOPipe::lookupStatusString(status) << ", " << (void*)pump << ")");
 	LLMemType m1(LLMemType::MTYPE_IO_URL_REQUEST);
 
-	if(!hasNotExpired())
+	if (LL_LIKELY(!mDetail->mCurlEasyRequest.isBuffered()))	// Currently always true.
 	{
+		// The last reference will be deleted when the pump that this chain belongs to
+		// is removed from the running chains vector, upon returning from this function.
+		// This keeps the CurlEasyRequest object alive until the curl thread cleanly removed it.
+		Dout(dc::curl, "Calling mDetail->mCurlEasyRequest.removeRequest()");
+		mDetail->mCurlEasyRequest.removeRequest();
+	}
+	else if (!hasNotExpired())
+	{
+		// The buffered version has it's own time out handling, and that already expired,
+		// so we can ignore the expiration of this timer (currently never happens).
+		// I left it here because it's what LL did (in the form if (!isValid() ...),
+		// and it would be relevant if this characteristic of mDetail->mCurlEasyRequest
+		// would change. --Aleric
 		return STATUS_EXPIRED ;
 	}