Merge lp:~thomas-voss/net-cpp/bug-fixing-and-features-landing into lp:net-cpp

[Thomas Voß]

* Enable pause/resume of requests.
* Fix LP:#1570686 and LP:#1570687

* Enable pause/resume of requests.
* Fix LP:#1570686 and LP:#1570687

[Thomas Voß (thomas-voss)]

@Gary: I changed a few minor things when compared to your MP:
  - pause/resume on the easy handle is invoked on the downloading/uploading thread now.
  - fixed the test case to talk to a server that is (hopefully) reachable from within our CI labs.
  - some packaging updates to account for the major version bump.
  - moved speed.{limit, duration} into the Request::Configuration struct, picking reasonable values by default.

[Gary.Wang (gary-wzl77)]

Thanks tvoss for this MP, it looks good to me.
In this MP, I didn't see pause/resume issue anymore.
I think i should put speed.{limit, duration} into Request::Configuration struct in my MP just as what you do in this MP since we already have this struct there.
I really like the implementation of progress bar, it's quite neat and simple.
Thanks again.

P.S I forget we have sth available in CI labs. :)

review: Approve

[lp:~thomas-voss/net-cpp/bug-fixing-and-features-landing]

55. By Thomas Voß on 2016-06-01

Make sure that exceptions while pausing/resuming an easy handle are not propagating.
Flush output stream for progress bar.

=== modified file 'debian/changelog'
--- debian/changelog	2015-04-15 12:34:49 +0000
+++ debian/changelog	2016-06-01 08:27:01 +0000
@@ -1,3 +1,11 @@
+net-cpp (2.0.0) UNRELEASED; urgency=medium
+
+  [Gary Wang]
+  * Enable pause/resume of requests.
+  * Fix LP:#1570686 and LP:#1570687
+
+ -- Thomas Voß <[e-mail]>  Tue, 31 May 2016 22:49:32 +0200
+
 net-cpp (1.2.0+15.04.20150415.2-0ubuntu1) vivid; urgency=medium
 
   [ Thomas Voß ]

=== modified file 'debian/control'
--- debian/control	2015-02-25 10:27:29 +0000
+++ debian/control	2016-06-01 08:27:01 +0000
@@ -29,7 +29,7 @@
 Vcs-Bzr: https://code.launchpad.net/~phablet-team/net-cpp/trunk
 Vcs-Browser: https://bazaar.launchpad.net/~phablet-team/net-cpp/trunk/files
 
-Package: libnet-cpp1
+Package: libnet-cpp2
 Architecture: any
 Multi-Arch: same
 Pre-Depends: ${misc:Pre-Depends},
@@ -45,7 +45,7 @@
 Architecture: any
 Multi-Arch: same
 Pre-Depends: ${misc:Pre-Depends},
-Depends: libnet-cpp1 (= ${binary:Version}),
+Depends: libnet-cpp2 (= ${binary:Version}),
          ${misc:Depends},
 Description: C++11 library for networking purposes - runtime library
  Net-Cpp is a simple and straightforward networking library for C++11.

=== renamed file 'debian/libnet-cpp1.install' => 'debian/libnet-cpp2.install'
=== renamed file 'debian/libnet-cpp1.symbols' => 'debian/libnet-cpp2.symbols'
--- debian/libnet-cpp1.symbols	2015-03-23 16:24:27 +0000
+++ debian/libnet-cpp2.symbols	2016-06-01 08:27:01 +0000
@@ -1,4 +1,4 @@
-libnet-cpp.so.1 libnet-cpp1 #MINVER#
+libnet-cpp.so.2 libnet-cpp2 #MINVER#
  (c++)"core::net::http::make_client()@Base" 0.0.1+14.10.20140611
  (c++)"core::net::http::make_streaming_client()@Base" 1.1.0+15.04.20150305-0ubuntu1
  (c++)"core::net::make_uri(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, std::vector<std::pair<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const&)@Base" 1.1.0+14.10.20140804

=== modified file 'include/core/net/http/request.h'
--- include/core/net/http/request.h	2016-06-01 08:27:01 +0000
+++ include/core/net/http/request.h	2016-06-01 08:27:01 +0000
@@ -213,6 +213,13 @@
             /** Invoked for querying user credentials to authenticate proxy accesses. */
             AuthenicationHandler for_proxy;
         } authentication_handler;
+
+        /** Encapsulates thresholds for minimum transfer speed in [kB/s] for duration seconds. */
+        struct
+        {
+            std::uint64_t limit{1};
+            std::chrono::seconds duration{std::chrono::seconds{30}};
+        } speed;
     };
 
     Request(const Request&) = delete;
@@ -249,7 +256,8 @@
     virtual void async_execute(const Handler& handler) = 0;
 
     /**
-     * @brief Pause the request
+     * @brief Pause the request with options for aborting the request.
+     * The request will be aborted if transfer speed falls below \a limit in [bytes/second] for \a time seconds.
      * @throw core::net::http::Error in case of http-related errors.
      */
     virtual void pause() = 0;
@@ -272,14 +280,6 @@
      */
     virtual std::string url_unescape(const std::string& s) = 0;
 
-    /**
-     * @brief Sets options for aborting the request.
-     * The request will be aborted if transfer speed belows \a limit bytes per second for \a time seconds
-     * @param limit The transfer speed in seconds.
-     * @param time waiting period(seconds) to abort the request.
-     */
-    virtual void abort_request_if(std::uint64_t limit, const std::chrono::seconds& time) = 0;
-
 protected:
     /** @cond */
     Request() = default;

=== modified file 'include/core/net/http/streaming_request.h'
--- include/core/net/http/streaming_request.h	2016-06-01 08:27:01 +0000
+++ include/core/net/http/streaming_request.h	2016-06-01 08:27:01 +0000
@@ -53,18 +53,6 @@
      * @return The response to the request.
      */
     virtual void async_execute(const Handler& handler, const DataHandler& dh) = 0;
-
-    /**
-    * @brief Pause the request
-    * @throw core::net::http::Error in case of http-related errors.
-    */
-    virtual void pause() = 0;
-  
-    /**
-    * @brief Resume the request
-    * @throw core::net::http::Error in case of http-related errors.
-    */
-    virtual void resume() = 0;
 };
 }
 }

=== modified file 'src/core/net/http/impl/curl/client.cpp'
--- src/core/net/http/impl/curl/client.cpp	2016-06-01 08:27:01 +0000
+++ src/core/net/http/impl/curl/client.cpp	2016-06-01 08:27:01 +0000
@@ -36,6 +36,11 @@
 namespace
 {
 const std::string BASE64_PADDING[] = { "", "==", "=" };
+template<typename To, typename From>
+To cast_without_overflow(From from)
+{
+    return static_cast<To>(std::min<From>(std::numeric_limits<To>::max(), from));
+}
 }
 
 http::impl::curl::Client::Client()
@@ -121,6 +126,8 @@
                       configuration.ssl.verify_host ? ::curl::easy::enable_ssl_host_verification : ::curl::easy::disable);
     handle.set_option(::curl::Option::ssl_verify_peer,
                       configuration.ssl.verify_peer ? ::curl::easy::enable : ::curl::easy::disable);
+    handle.set_option(::curl::Option::low_speed_limit, cast_without_overflow<long>(configuration.speed.limit));
+    handle.set_option(::curl::Option::low_speed_time, cast_without_overflow<long>(configuration.speed.duration.count()));
 
     if (configuration.authentication_handler.for_http)
     {
@@ -142,6 +149,8 @@
                       configuration.ssl.verify_host ? ::curl::easy::enable_ssl_host_verification : ::curl::easy::disable);
     handle.set_option(::curl::Option::ssl_verify_peer,
                       configuration.ssl.verify_peer ? ::curl::easy::enable : ::curl::easy::disable);
+    handle.set_option(::curl::Option::low_speed_limit, cast_without_overflow<long>(configuration.speed.limit));
+    handle.set_option(::curl::Option::low_speed_time, cast_without_overflow<long>(configuration.speed.duration.count()));
 
     if (configuration.authentication_handler.for_http)
     {
@@ -167,6 +176,8 @@
                       configuration.ssl.verify_host ? ::curl::easy::enable_ssl_host_verification : ::curl::easy::disable);
     handle.set_option(::curl::Option::ssl_verify_peer,
                       configuration.ssl.verify_peer ? ::curl::easy::enable : ::curl::easy::disable);
+    handle.set_option(::curl::Option::low_speed_limit, cast_without_overflow<long>(configuration.speed.limit));
+    handle.set_option(::curl::Option::low_speed_time, cast_without_overflow<long>(configuration.speed.duration.count()));
 
     if (configuration.authentication_handler.for_http)
     {
@@ -200,6 +211,8 @@
                       configuration.ssl.verify_host ? ::curl::easy::enable_ssl_host_verification : ::curl::easy::disable);
     handle.set_option(::curl::Option::ssl_verify_peer,
                       configuration.ssl.verify_peer ? ::curl::easy::enable : ::curl::easy::disable);
+    handle.set_option(::curl::Option::low_speed_limit, cast_without_overflow<long>(configuration.speed.limit));
+    handle.set_option(::curl::Option::low_speed_time, cast_without_overflow<long>(configuration.speed.duration.count()));
 
     if (configuration.authentication_handler.for_http)
     {
@@ -231,6 +244,8 @@
                       configuration.ssl.verify_host ? ::curl::easy::enable_ssl_host_verification : ::curl::easy::disable);
     handle.set_option(::curl::Option::ssl_verify_peer,
                       configuration.ssl.verify_peer ? ::curl::easy::enable : ::curl::easy::disable);
+    handle.set_option(::curl::Option::low_speed_limit, cast_without_overflow<long>(configuration.speed.limit));
+    handle.set_option(::curl::Option::low_speed_time, cast_without_overflow<long>(configuration.speed.duration.count()));
 
     if (configuration.authentication_handler.for_http)
     {
@@ -252,6 +267,8 @@
                       configuration.ssl.verify_host ? ::curl::easy::enable_ssl_host_verification : ::curl::easy::disable);
     handle.set_option(::curl::Option::ssl_verify_peer,
                       configuration.ssl.verify_peer ? ::curl::easy::enable : ::curl::easy::disable);
+    handle.set_option(::curl::Option::low_speed_limit, cast_without_overflow<long>(configuration.speed.limit));
+    handle.set_option(::curl::Option::low_speed_time, cast_without_overflow<long>(configuration.speed.duration.count()));
 
     if (configuration.authentication_handler.for_http)
     {

=== modified file 'src/core/net/http/impl/curl/easy.cpp'
--- src/core/net/http/impl/curl/easy.cpp	2016-06-01 08:27:01 +0000
+++ src/core/net/http/impl/curl/easy.cpp	2016-06-01 08:27:01 +0000
@@ -451,7 +451,7 @@
 void easy::Handle::resume()
 {
     if (!d) throw easy::Handle::HandleHasBeenAbandoned{};
-    throw_if_not<curl::Code::ok>(easy::native::pause(native(), CURLPAUSE_RECV_CONT), [this]() { return std::string{d->error};});
+    throw_if_not<curl::Code::ok>(easy::native::pause(native(), CURLPAUSE_CONT), [this]() { return std::string{d->error};});
 }
 
 // URL escapes the given input string.

=== modified file 'src/core/net/http/impl/curl/multi.cpp'
--- src/core/net/http/impl/curl/multi.cpp	2015-02-25 10:26:38 +0000
+++ src/core/net/http/impl/curl/multi.cpp	2016-06-01 08:27:01 +0000
@@ -285,6 +285,11 @@
     return result;
 }
 
+void multi::Handle::dispatch(const std::function<void ()> &task)
+{
+    d->dispatcher.post(task);
+}
+
 void multi::Handle::run()
 {
     d->dispatcher.run();

=== modified file 'src/core/net/http/impl/curl/multi.h'
--- src/core/net/http/impl/curl/multi.h	2014-03-13 20:50:36 +0000
+++ src/core/net/http/impl/curl/multi.h	2016-06-01 08:27:01 +0000
@@ -140,6 +140,9 @@
     // Queries statistics about the timing information of the last transfers.
     core::net::http::Client::Timings timings();
 
+    // Dispatch dispatches task on the underlying reactor.
+    void dispatch(const std::function<void()>& task);
+
     // Executes the underlying dispatcher executing the curl multi instance.
     // Can be called multiple times for thread-pool use-cases.
     void run();

=== modified file 'src/core/net/http/impl/curl/request.h'
--- src/core/net/http/impl/curl/request.h	2016-06-01 08:27:01 +0000
+++ src/core/net/http/impl/curl/request.h	2016-06-01 08:27:01 +0000
@@ -242,7 +242,6 @@
                 case Request::Progress::Next::abort_operation: result = 1; break;
                 case Request::Progress::Next::continue_operation: result = 0; break;
                 }
-
                 return result;
             });
         }
@@ -275,26 +274,32 @@
     }
 
     void pause()
-    {   
-        try
-        {   
-            easy.pause();
-        } catch(const std::system_error& se)
-        {   
-            throw core::net::http::Error(se.what(), CORE_FROM_HERE());
-        }       
+    {
+        auto copy = easy;
+        multi.dispatch([copy]() mutable
+        {
+            try
+            {
+                copy.pause();
+            }
+            catch(...) {}
+        });
+
     }       
 
     void resume()
-    {   
-        try
-        {   
-            easy.resume();
-        } catch(const std::system_error& se)
-        {   
-            throw core::net::http::Error(se.what(), CORE_FROM_HERE());
-        }       
-    }       
+    {
+        auto copy = easy;
+        multi.dispatch([copy]() mutable
+        {
+            try
+            {
+                copy.resume();
+            }
+            catch(...) {}
+
+        });
+    }
 
     std::string url_escape(const std::string& s)
     {
@@ -306,15 +311,6 @@
         return easy.unescape(s);
     }
 
-    void abort_request_if(std::uint64_t limit, const std::chrono::seconds& time)
-    {
-        if (atomic_state.load() != core::net::http::Request::State::ready)
-            throw core::net::http::Request::Errors::AlreadyActive{CORE_FROM_HERE()};
-    
-        easy.set_option(::curl::Option::low_speed_limit, limit);
-        easy.set_option(::curl::Option::low_speed_time, time.count());
-    }
-
 private:
     std::atomic<core::net::http::Request::State> atomic_state;
     ::curl::multi::Handle multi;

=== modified file 'tests/http_streaming_client_test.cpp'
--- tests/http_streaming_client_test.cpp	2016-06-01 08:27:01 +0000
+++ tests/http_streaming_client_test.cpp	2016-06-01 08:27:01 +0000
@@ -35,6 +35,8 @@
 #include <memory>
 
 #include <fstream>
+#include <iomanip>
+#include <iostream>
 
 namespace http = core::net::http;
 namespace json = Json;
@@ -67,16 +69,43 @@
     MockDataHandler() = default;
 };
 
-auto default_progress_reporter = [](const http::Request::Progress& progress)
-{
-    if (progress.download.current > 0. && progress.download.total > 0.)
-        std::cout << "Download progress: " << progress.download.current / progress.download.total << std::endl;
-    if (progress.upload.current > 0. && progress.upload.total > 0.)
-        std::cout << "Upload progress: " << progress.upload.current / progress.upload.total << std::endl;
-
+class ProgressBar
+{
+public:
+    ProgressBar(std::uint32_t width)
+        : width{width}, out{std::cout}
+    {
+    }
+
+    ~ProgressBar()
+    {
+        out << std::endl;
+    }
+
+    void update(double percentage)
+    {
+        struct CursorState
+        {
+            CursorState(std::ostream& out) : out{out} { out << "\33[?25l"; }
+            ~CursorState()                            { out << "\33[?25h"; }
+            std::ostream& out;
+        } cs{out};
+
+        out << "\r" << "[" << std::setw(width) << std::left << std::setfill(' ') << std::string(percentage * width, '=') << "] " << std::setw(5) << std::fixed << std::setprecision(2) << percentage * 100 << " %" << std::flush;
+    }
+
+private:
+    std::uint32_t width;
+    std::ostream& out;
+};
+
+auto silent_progress_reporter = [](const http::Request::Progress&)
+{
     return http::Request::Progress::Next::continue_operation;
 };
 
+auto default_progress_reporter = silent_progress_reporter;
+
 bool init()
 {
     static httpbin::Instance instance;
@@ -594,3 +623,61 @@
     EXPECT_EQ(url, root["url"].asString());
 }
 
+TEST(StreamingHttpClient, request_can_be_paused_and_resumed)
+{
+    using namespace ::testing;
+    // We obtain a default client instance, dispatching to the default implementation.
+    auto client = http::make_streaming_client();
+
+    // Execute the client
+    std::thread worker{[client]() { client->run(); }};
+
+    auto url = "http://archive.ubuntu.com/ubuntu/dists/xenial/main/installer-amd64/current/images/netboot/mini.iso";
+
+    // The client mostly acts as a factory for http requests.
+    auto request = client->streaming_get(http::Request::Configuration::from_uri_as_string(url));
+
+    // Our mocked data handler.
+    auto dh = MockDataHandler::create(); EXPECT_CALL(*dh, on_new_data(_)).Times(AtLeast(1));
+
+    std::promise<core::net::http::Response> promise;
+    auto future = promise.get_future();
+
+    ProgressBar pb{80};
+
+    // We finally execute the query asynchronously.
+    request->async_execute(http::Request::Handler()
+        .on_progress([&pb](const http::Request::Progress& progress)
+        {
+            if (progress.download.current > 0. && progress.download.total > 0.)
+                pb.update(progress.download.current / static_cast<double>(progress.download.total));
+            return http::Request::Progress::Next::continue_operation;
+        })
+        .on_response([&](const core::net::http::Response& response)
+        {
+            promise.set_value(response);
+        })
+        .on_error([&](const core::net::Error& e)
+        {
+            promise.set_exception(std::make_exception_ptr(e));
+        }),
+        dh->to_data_handler());
+
+    std::this_thread::sleep_for(std::chrono::seconds(2));
+
+    request->pause();
+    request->resume();
+
+    try
+    {
+        // This might very well throw.
+        EXPECT_EQ(core::net::http::Status::ok, future.get().status);
+    } catch (const std::exception& e) { FAIL() << e.what(); }
+
+    client->stop();
+
+    // We shut down our worker thread
+    if (worker.joinable())
+        worker.join();    
+}
+