From 8c522aa758bb19c4738ba48559a6f05e0aae9b06 Mon Sep 17 00:00:00 2001 From: Vinnie Falco Date: Wed, 11 Sep 2013 21:38:30 -0700 Subject: [PATCH] Fix off by one in pending i/o count on HTTPClient --- modules/beast_asio/http/HTTPClientType.cpp | 111 +++++++++++++-------- 1 file changed, 70 insertions(+), 41 deletions(-) diff --git a/modules/beast_asio/http/HTTPClientType.cpp b/modules/beast_asio/http/HTTPClientType.cpp index 143b19fdb..d1e1dd98e 100644 --- a/modules/beast_asio/http/HTTPClientType.cpp +++ b/modules/beast_asio/http/HTTPClientType.cpp @@ -92,7 +92,6 @@ public: boost::asio::io_service io_service; async_get (io_service, nullptr, url); io_service.run (); - cancel (); return result (); } @@ -224,6 +223,7 @@ private: , m_context (boost::asio::ssl::context::sslv23) , m_buffer (bufferSize) , m_parser (HTTPParser::typeResponse) + , m_timer_set (false) , m_timer_canceled (false) , m_timer_expired (false) , m_messageLimitBytes (messageLimitBytes) @@ -241,6 +241,7 @@ private: m_timer.expires_from_now ( boost::posix_time::milliseconds ( long (timeoutSeconds * 1000))); + m_timer_set = true; ++m_io_pending; m_timer.async_wait (TimerHandler (this)); @@ -261,7 +262,11 @@ private: // void cancel () { - cancel_all (); + cancel_timer (); + m_resolver.cancel (); + error_code ec; + m_socket.close (ec); + m_done.wait (); } @@ -270,7 +275,7 @@ private: // Counts a pending i/o as canceled // - void cancel_io () + void io_canceled () { bassert (m_io_pending.get () > 0); if (--m_io_pending == 0) @@ -281,22 +286,41 @@ private: // void cancel_timer () { - m_timer_canceled = true; - error_code ec; - m_timer.cancel (ec); + // Make sure the timer was set (versus infinite timeout) + if (m_timer_set) + { + // See if it was already canceled. + if (! m_timer_canceled) + { + m_timer_canceled = true; + error_code ec; + m_timer.cancel (ec); + + // At this point, there will either be a pending completion + // or a pending abort for the handler. If its a completion, + // they will see that the timer was canceled (since we're on + // a strand, everything is serialized). If its an abort it + // counts as a cancellation anyway. Either way, we will deduct + // one i/o from the pending i/o count. + } + } } // Called to notify the original handler the operation is complete. // void complete (error_code const& ec) { + // Set the error code in the result. m_owner.m_result.error = ec; + // Cancel the deadline timer. This ensures that + // we will not return 'timeout' to the caller later. + // cancel_timer (); bassert (m_io_pending.get () > 0); - cancel_io (); + io_canceled (); // We call the handler directly since we know // we are already in the right context, and @@ -314,62 +338,66 @@ private: if (m_timer_expired || ec == boost::asio::error::operation_aborted) { - cancel_io (); + // Timer expired, or the operation was aborted due to + // cancel, so we deduct one i/o and return immediately. + // + io_canceled (); return true; } - else if (ec != 0 && ec != boost::asio::error::eof) + + if (ec != 0 && ec != boost::asio::error::eof) { + // A real error happened, and the timer didn't expire, so + // notify the original handler that the operation is complete. + // complete (ec); return true; } + // Process the completion as usual. If the caller does not + // call another initiating function, it is their responsibility + // to call io_canceled() to deduce one pending i/o. + // return false; } - // Cancels/closes all i/o objects. - // - void cancel_all () - { - cancel_timer (); - m_resolver.cancel (); - error_code ec; - m_socket.close (ec); - } - // Called when the deadline timer expires or is canceled. // void timerCompletion (error_code ec) { - if (ec == boost::asio::error::operation_aborted) + bassert (m_timer_set); + + if (m_timer_canceled || ec == boost::asio::error::operation_aborted) { - bassert (m_timer_canceled); - return cancel_io (); + // If the cancel flag is set or the operation was aborted it + // means we canceled the timer so deduct one i/o and return. + // + io_canceled (); + return; } - check_invariant (ec == 0); + bassert (ec == 0); - // Handle the case where the timer completion has already - // been queued for dispatch but we have finished the operation - // and queued the completion handler for dispatch. + // The timer expired, so this is a real timeout scenario. + // We want to set the error code, notify the handler, and cancel + // all other pending i/o. // - if (! m_timer_canceled) - { - m_timer_expired = true; + m_timer_expired = true; - ec = error_code (boost::asio::error::timed_out, - boost::asio::error::get_system_category ()); + ec = error_code (boost::asio::error::timed_out, + boost::asio::error::get_system_category ()); - complete (ec); + // Cancel pending name resolution + m_resolver.cancel (); - io_complete (ec); + // Close the socket. This will cancel up to 2 pending i/o + m_socket.close (ec); - m_resolver.cancel (); - m_socket.close (ec); - } - else - { - cancel_io (); - } + // Notify the original handler of a timeout error. + // The call to complete() consumes one pending i/o, which + // we need since this function counts as one completion. + // + complete (ec); } //---------------------------------------------------------------------- @@ -594,7 +622,7 @@ private: } // deduct one i/o since we aren't issuing any new one - cancel_io (); + io_canceled (); } void read_complete (error_code ec, std::size_t bytes_transferred) @@ -671,6 +699,7 @@ private: State m_state; HTTPParser m_parser; String m_get_string; + bool m_timer_set; bool m_timer_canceled; bool m_timer_expired; std::size_t m_messageLimitBytes;