From 8596b0b9eaf7fb5edc52afb2931bd7c439fb465f Mon Sep 17 00:00:00 2001 From: Matthias Koefferlein Date: Tue, 1 May 2018 21:24:05 +0200 Subject: [PATCH] libcurl integration and HTTP support enhanced Issue was: the asynch interface for HTTP access wasn't entirely compatible between Curl and Qt implementations. Plus, the progress integration was messed up because of the deleteLater scheme (progress was visible even after the connection ended). The deleteLater scheme was replaced by an explicit close which does not require the connection to be deleted immediately in the receivers. --- src/lay/lay/laySaltManagerDialog.cc | 20 +++++-- src/tl/tl/tlHttpStreamCurl.cc | 89 ++++++++++++++++++++++++----- src/tl/tl/tlHttpStreamCurl.h | 33 +++++++++-- src/tl/tl/tlHttpStreamQt.cc | 12 +++- src/tl/tl/tlHttpStreamQt.h | 7 +++ src/tl/tl/tlStream.cc | 58 ++++++++++++------- src/tl/tl/tlStream.h | 17 ++++++ src/tl/tl/tlXMLParser.cc | 6 +- 8 files changed, 197 insertions(+), 45 deletions(-) diff --git a/src/lay/lay/laySaltManagerDialog.cc b/src/lay/lay/laySaltManagerDialog.cc index c6d4c5f34..7c2195ecf 100644 --- a/src/lay/lay/laySaltManagerDialog.cc +++ b/src/lay/lay/laySaltManagerDialog.cc @@ -687,7 +687,10 @@ void SaltManagerDialog::salt_mine_download_finished () { QApplication::restoreOverrideCursor (); - m_salt_mine_reader.reset (0); + if (m_salt_mine_reader.get ()) { + // NOTE: don't delete the reader in the slot it triggered + m_salt_mine_reader->close (); + } } void @@ -951,7 +954,10 @@ SaltManagerDialog::get_remote_grain_info (lay::SaltGrain *g, SaltGrainDetailsTex } m_downloaded_grain.reset (0); - m_downloaded_grain_reader.reset (0); + if (m_downloaded_grain_reader.get ()) { + // NOTE: don't delete the reader in the slot it triggered + m_downloaded_grain_reader->close (); + } mp_downloaded_target = details; m_salt_mine_grain.reset (new lay::SaltGrain (*g)); @@ -1022,7 +1028,10 @@ SaltManagerDialog::data_ready () mp_downloaded_target->set_grain (m_downloaded_grain.get ()); m_downloaded_grain.reset (0); - m_downloaded_grain_reader.reset (0); + if (m_downloaded_grain_reader.get ()) { + // NOTE: don't delete the reader in the slot it triggered + m_downloaded_grain_reader->close (); + } m_salt_mine_grain.reset (0); } catch (tl::Exception &ex) { @@ -1049,7 +1058,10 @@ SaltManagerDialog::show_error (tl::Exception &ex) mp_downloaded_target->setHtml (html); m_downloaded_grain.reset (0); - m_downloaded_grain_reader.reset (0); + if (m_downloaded_grain_reader.get ()) { + // NOTE: don't delete the reader in the slot it triggered + m_downloaded_grain_reader->close(); + } m_salt_mine_grain.reset (0); } diff --git a/src/tl/tl/tlHttpStreamCurl.cc b/src/tl/tl/tlHttpStreamCurl.cc index 5d73b222d..9bfdb1e5a 100644 --- a/src/tl/tl/tlHttpStreamCurl.cc +++ b/src/tl/tl/tlHttpStreamCurl.cc @@ -406,6 +406,11 @@ public: */ void send (); + /** + * @brief Closes this connection + */ + void close (); + /** * @brief Gets the HTTP status after the finished_event has been triggered */ @@ -568,6 +573,19 @@ CurlConnection::CurlConnection (const CurlConnection &other) init (); } +void CurlConnection::close () +{ + CurlNetworkManager::instance ()->release_connection (this); + curl_slist_free_all (mp_headers); + + mp_handle = 0; + m_http_status = 0; + m_finished = false; + m_status = 0; + mp_headers = 0; + m_authenticated = 0; +} + void CurlConnection::init () { m_http_status = 0; @@ -584,8 +602,10 @@ CurlConnection::~CurlConnection () #if defined(DEBUG_CURL) std::cerr << "~CurlConnection(" << (void *)mp_handle << ")" << std::endl; #endif - CurlNetworkManager::instance ()->release_connection (this); - curl_slist_free_all (mp_headers); + if (mp_handle) { + CurlNetworkManager::instance ()->release_connection (this); + curl_slist_free_all (mp_headers); + } } void CurlConnection::set_url (const char *url) @@ -648,6 +668,8 @@ size_t CurlConnection::fetch_data (char *buffer, size_t nbytes) void CurlConnection::send () { + tl_assert (mp_handle != 0); + m_http_status = 0; m_status = 0; m_finished = false; @@ -764,6 +786,8 @@ void CurlConnection::check () const void CurlConnection::finished (int status) { + tl_assert (mp_handle != 0); + if (status != 0) { m_status = status; m_finished = true; @@ -1058,10 +1082,12 @@ int CurlNetworkManager::tick () InputHttpStream::InputHttpStream (const std::string &url) { m_sent = false; + m_ready = false; m_connection.reset (CurlNetworkManager::instance ()->create_connection ()); m_connection->set_url (url.c_str ()); - m_connection->finished_event.add (this, &InputHttpStream::finished); + m_connection->data_available_event.add (this, &InputHttpStream::on_data_available); + m_connection->finished_event.add (this, &InputHttpStream::on_finished); } InputHttpStream::~InputHttpStream () @@ -1100,46 +1126,79 @@ InputHttpStream::add_header (const std::string &name, const std::string &value) } void -InputHttpStream::finished () +InputHttpStream::on_finished () { + m_progress.reset (0); m_ready_event (); } +void +InputHttpStream::on_data_available () +{ + // send the ready event just once + if (! m_ready) { + m_data_ready_event (); + m_ready = true; + } +} + void InputHttpStream::send () { + m_ready = false; + m_progress.reset (0); m_connection->send (); m_sent = true; } +void +InputHttpStream::check () +{ + if (m_connection->finished ()) { + m_connection->check (); + } +} + size_t InputHttpStream::read (char *b, size_t n) { if (! m_sent) { - send (); + } + // block until enough data is available + { // Prevents deferred methods to be executed during the processEvents below (undesired side effects) tl::NoDeferredMethods silent; - // TODO: progress, timeout - tl::AbsoluteProgress progress (tl::to_string (QObject::tr ("Downloading")) + " " + m_connection->url (), 1); - while (! m_connection->finished () && CurlNetworkManager::instance ()->tick ()) { - ++progress; + if (! m_progress.get ()) { + m_progress.reset (new tl::AbsoluteProgress (tl::to_string (QObject::tr ("Downloading")) + " " + m_connection->url (), 1)); } - tl_assert (m_connection->finished ()); + while (n > m_connection->read_available () && ! m_connection->finished () && CurlNetworkManager::instance ()->tick ()) { + ++*m_progress; + } + } + + if (m_connection->finished ()) { m_connection->check (); - - if (tl::verbosity() >= 40) { - tl::info << "HTTP reponse data read: " << m_connection->read_data_to_string (); - } - + } else if (tl::verbosity() >= 40) { + tl::info << "HTTP reponse data read: " << m_connection->read_data_to_string (); } return m_connection->fetch_read_data (b, n); } +void +InputHttpStream::close () +{ + m_progress.reset (0); + if (m_connection.get ()) { + m_connection->close (); + } + m_sent = m_ready = false; +} + void InputHttpStream::reset () { diff --git a/src/tl/tl/tlHttpStreamCurl.h b/src/tl/tl/tlHttpStreamCurl.h index 4d7abf3d0..0424e035d 100644 --- a/src/tl/tl/tlHttpStreamCurl.h +++ b/src/tl/tl/tlHttpStreamCurl.h @@ -28,6 +28,7 @@ #include "tlStream.h" #include "tlEvents.h" #include "tlObject.h" +#include "tlProgress.h" #include @@ -43,8 +44,7 @@ class HttpCredentialProvider; * Implements the reader from a server using the HTTP protocol */ class TL_PUBLIC InputHttpStream - // NOTE: QObject is required because we use "deleteLater" - : public QObject, public tl::Object, public InputStreamBase + : public tl::Object, public InputStreamBase { public: /** @@ -111,19 +111,40 @@ public: /** * @brief Gets the "ready" event * Connect to this event for the asynchroneous interface. + * This event is fired when the request has finished. */ tl::Event &ready () { return m_ready_event; } + /** + * @brief Checks for errors + * This method can be used after the ready event to check for errors. + * It will throw an exception if errors occured. + * read() will do the same. + */ + void check (); + + /** + * @brief Gets the "data available" event + * Connect to this event for the asynchroneous interface. + * This event is fired when data becomes available for read. + * It is just fired once. + */ + tl::Event &data_ready () + { + return m_data_ready_event; + } + /** * @brief Gets a value indicating whether data is available */ bool data_available (); + // Basic interface virtual void reset (); - + virtual void close (); virtual std::string source () const; virtual std::string absolute_path () const; virtual std::string filename () const; @@ -131,9 +152,13 @@ public: private: std::auto_ptr m_connection; tl::Event m_ready_event; + tl::Event m_data_ready_event; bool m_sent; + bool m_ready; + std::auto_ptr m_progress; - void finished (); + void on_data_available (); + void on_finished (); }; } diff --git a/src/tl/tl/tlHttpStreamQt.cc b/src/tl/tl/tlHttpStreamQt.cc index 796e6caab..c591f6ca1 100644 --- a/src/tl/tl/tlHttpStreamQt.cc +++ b/src/tl/tl/tlHttpStreamQt.cc @@ -115,7 +115,17 @@ InputHttpStream::InputHttpStream (const std::string &url) InputHttpStream::~InputHttpStream () { - // .. nothing yet .. + close (); +} + +void +InputHttpStream::close () +{ + if (mp_active_reply.get ()) { + mp_active_reply->abort (); + mp_active_reply.release ()->deleteLater (); + } + mp_reply = 0; } void diff --git a/src/tl/tl/tlHttpStreamQt.h b/src/tl/tl/tlHttpStreamQt.h index f2c2b8e62..7fcb78894 100644 --- a/src/tl/tl/tlHttpStreamQt.h +++ b/src/tl/tl/tlHttpStreamQt.h @@ -102,6 +102,11 @@ public: */ void send (); + /** + * @brief Closes the connection + */ + void close (); + /** * @brief Sets the request verb * The default verb is "GET" @@ -136,6 +141,8 @@ public: /** * @brief Gets the "ready" event * Connect to this event for the asynchroneous interface. + * This event is fired when data becomes available or the + * connection has terminated with an error. */ tl::Event &ready () { diff --git a/src/tl/tl/tlStream.cc b/src/tl/tl/tlStream.cc index 23def2170..90b804ec2 100644 --- a/src/tl/tl/tlStream.cc +++ b/src/tl/tl/tlStream.cc @@ -161,6 +161,8 @@ public: virtual void reset (); + virtual void close (); + virtual std::string source () const { return m_source; @@ -203,19 +205,12 @@ public: */ virtual ~InputFile (); - /** - * @brief Read from a file - * - * Implements the basic read method. - * Will throw a FileReadErrorException if an error occurs. - */ virtual size_t read (char *b, size_t n); - /** - * @brief Reset to the beginning of the file - */ virtual void reset (); + virtual void close (); + virtual std::string source () const { return m_source; @@ -272,6 +267,11 @@ public: */ virtual void reset (); + /** + * @brief Closes the pipe + */ + virtual void close (); + /** * @brief Get the source specification (the file name) * @@ -365,15 +365,7 @@ std::string InputStream::absolute_path (const std::string &abstract_path) InputStream::~InputStream () { if (mp_delegate && m_owns_delegate) { - // NOTE: HTTP stream objects should not be deleted now, since events - // may be pending that deliver the finished signal to the object. - tl::InputHttpStream *http = dynamic_cast(mp_delegate); - if (http) { - http->ready ().clear (); // avoids events from deleted streams - http->deleteLater (); - } else { - delete mp_delegate; - } + delete mp_delegate; mp_delegate = 0; } if (mp_inflate) { @@ -502,6 +494,14 @@ InputStream::inflate () mp_inflate = new tl::InflateFilter (*this); } +void +InputStream::close () +{ + if (mp_delegate) { + mp_delegate->close (); + } +} + void InputStream::reset () { @@ -649,12 +649,18 @@ InputFile::InputFile (const std::string &path) } InputFile::~InputFile () +{ + close (); +} + +void +InputFile::close () { if (m_fd >= 0) { #if defined(_WIN32) _close (m_fd); #else - close (m_fd); + ::close (m_fd); #endif m_fd = -1; } @@ -723,6 +729,12 @@ InputZLibFile::InputZLibFile (const std::string &path) } InputZLibFile::~InputZLibFile () +{ + close (); +} + +void +InputZLibFile::close () { if (m_zs != NULL) { gzclose (m_zs); @@ -1171,11 +1183,17 @@ InputPipe::InputPipe (const std::string &path) } InputPipe::~InputPipe () +{ + close (); +} + +void +InputPipe::close () { if (m_file != NULL) { fclose (m_file); m_file = NULL; - } + } } size_t diff --git a/src/tl/tl/tlStream.h b/src/tl/tl/tlStream.h index f88fb6990..373059af0 100644 --- a/src/tl/tl/tlStream.h +++ b/src/tl/tl/tlStream.h @@ -83,6 +83,11 @@ public: */ virtual void reset () = 0; + /** + * @brief Closes the channel + */ + virtual void close () = 0; + /** * @brief Get the source specification (i.e. the file name) */ @@ -136,6 +141,11 @@ public: m_pos = 0; } + virtual void close () + { + // .. nothing yet .. + } + virtual std::string source () const { return "data"; @@ -306,6 +316,13 @@ public: */ virtual void reset (); + /** + * @brief Closes the reader + * This method will finish reading and free resources + * associated with it. HTTP connections will be closed. + */ + void close (); + /** * @brief Gets the absolute path for a given URL */ diff --git a/src/tl/tl/tlXMLParser.cc b/src/tl/tl/tlXMLParser.cc index 3dc560bf7..6ac981bd6 100644 --- a/src/tl/tl/tlXMLParser.cc +++ b/src/tl/tl/tlXMLParser.cc @@ -106,7 +106,11 @@ public: *data++ = *rd; } - return n0 - n; + if (n0 == n) { + return -1; + } else { + return n0 - n; + } } catch (tl::Exception &ex) { setErrorString (tl::to_qstring (ex.msg ()));