From 80f2c2f40716acd17026a395ee6a09466adf75d9 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Tue, 25 Aug 2020 17:06:47 +0000 Subject: [PATCH] Reuse connection when calling ReadToCount() (#518) * change EOF for IsEOF function to avoid setting EOF after checking read everytime * fix setting size to read * move IsEOF to be private * Update comment about not moving a connection back to the pool --- sdk/core/azure-core/inc/http/curl/curl.hpp | 21 +++++++++-------- sdk/core/azure-core/src/http/curl/curl.cpp | 27 ++++++++-------------- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/sdk/core/azure-core/inc/http/curl/curl.hpp b/sdk/core/azure-core/inc/http/curl/curl.hpp index dc0cbae03..ceb3f7eb3 100644 --- a/sdk/core/azure-core/inc/http/curl/curl.hpp +++ b/sdk/core/azure-core/inc/http/curl/curl.hpp @@ -238,13 +238,6 @@ namespace Azure { namespace Core { namespace Http { */ int64_t m_uploadedBytes; - /** - * @brief Control field that gets true as soon as there is no more data to read from network. A - * network socket will return 0 once we got the entire reponse. - * - */ - bool m_rawResponseEOF; - /** * @brief Control field to handle the case when part of HTTP response body was copied to the * inner buffer. When a libcurl stream tries to read part of the body, this field will help to @@ -381,6 +374,12 @@ namespace Azure { namespace Core { namespace Http { */ int64_t ReadSocketToBuffer(uint8_t* buffer, int64_t bufferSize); + bool IsEOF() + { + return this->m_isChunkedResponseType ? this->m_chunkSize == 0 + : this->m_contentLength == this->m_sessionTotalRead; + } + public: #ifdef TESTING_BUILD // Makes possible to know the number of current connections in the connection pool @@ -401,17 +400,19 @@ namespace Azure { namespace Core { namespace Http { this->m_connection = GetCurlConnection(this->m_request); this->m_bodyStartInBuffer = -1; this->m_innerBufferSize = Details::c_DefaultLibcurlReaderSize; - this->m_rawResponseEOF = false; this->m_isChunkedResponseType = false; this->m_uploadedBytes = 0; + this->m_sessionTotalRead = 0; } ~CurlSession() override { // mark connection as reusable only if entire response was read // If not, connection can't be reused because next Read will start from what it is currently - // in the wire. We leave the connection blocked until Server closes the connection - if (this->m_rawResponseEOF) + // in the wire. + // By not moving the connection back to the pool, it gets destroyed calling the connection + // destructor to clean libcurl handle and close the connection. + if (IsEOF()) { MoveConnectionBackToPool(std::move(this->m_connection)); } diff --git a/sdk/core/azure-core/src/http/curl/curl.cpp b/sdk/core/azure-core/src/http/curl/curl.cpp index c15a1f84d..ffd9c9188 100644 --- a/sdk/core/azure-core/src/http/curl/curl.cpp +++ b/sdk/core/azure-core/src/http/curl/curl.cpp @@ -302,7 +302,6 @@ void CurlSession::ParseChunkSize() if (this->m_chunkSize == 0) { // Response with no content. end of chunk - this->m_rawResponseEOF = true; keepPolling = false; break; } @@ -367,7 +366,6 @@ void CurlSession::ReadStatusLineAndHeadersFromRawResponse() { this->m_contentLength = 0; this->m_bodyStartInBuffer = -1; - this->m_rawResponseEOF = true; return; } @@ -421,13 +419,13 @@ int64_t CurlSession::Read(Azure::Core::Context const& context, uint8_t* buffer, { context.ThrowIfCanceled(); - if (count <= 0 || this->m_rawResponseEOF) + if (count <= 0 || this->IsEOF()) { return 0; } // check if all chunked is all read already - if (this->m_isChunkedResponseType && this->m_chunkSize == 0) + if (this->m_isChunkedResponseType && this->m_chunkSize == this->m_sessionTotalRead) { // Need to read CRLF after all chunk was read for (int8_t i = 0; i < 2; i++) @@ -443,18 +441,21 @@ int64_t CurlSession::Read(Azure::Core::Context const& context, uint8_t* buffer, this->m_bodyStartInBuffer = 1; // jump first char (could be \r or \n) } } + // Reset session read counter for next chunk + this->m_sessionTotalRead = 0; // get the size of next chunk ParseChunkSize(); - if (this->m_rawResponseEOF) + if (this->IsEOF()) { // after parsing next chunk, check if it is zero return 0; } } auto totalRead = int64_t(); - auto readRequestLength - = this->m_isChunkedResponseType ? std::min(this->m_chunkSize, count) : count; + auto readRequestLength = this->m_isChunkedResponseType + ? std::min(this->m_chunkSize - this->m_sessionTotalRead, count) + : count; // For responses with content-length, avoid trying to read beyond Content-length or // libcurl could return a second response as BadRequest. @@ -476,10 +477,6 @@ int64_t CurlSession::Read(Azure::Core::Context const& context, uint8_t* buffer, totalRead = innerBufferMemoryStream.Read(context, buffer, readRequestLength); this->m_bodyStartInBuffer += totalRead; this->m_sessionTotalRead += totalRead; - if (this->m_isChunkedResponseType) - { - this->m_chunkSize -= totalRead; - } if (this->m_bodyStartInBuffer == this->m_innerBufferSize) { @@ -490,10 +487,8 @@ int64_t CurlSession::Read(Azure::Core::Context const& context, uint8_t* buffer, // Head request have contentLength = 0, so we won't read more, just return 0 // Also if we have already read all contentLength - if (this->m_sessionTotalRead == this->m_contentLength || this->m_rawResponseEOF) + if (this->m_sessionTotalRead == this->m_contentLength || this->IsEOF()) { - // make sure EOF for response is set to true - this->m_rawResponseEOF = true; return 0; } @@ -501,10 +496,6 @@ int64_t CurlSession::Read(Azure::Core::Context const& context, uint8_t* buffer, // For chunk request, read a chunk based on chunk size totalRead = ReadSocketToBuffer(buffer, static_cast(readRequestLength)); this->m_sessionTotalRead += totalRead; - if (this->m_isChunkedResponseType) - { - this->m_chunkSize -= totalRead; - } return totalRead; }