Merge lp:~marcustomlinson/net-cpp/fix_ppc_timeout into lp:net-cpp

[Marcus Tomlinson]

Explicitly cast milliseconds::count() to long.

Explicitly cast milliseconds::count() to long.

On PPC, this method returns an 8-byte long long, of which libCurl is hardcoded to read only the first 4 bytes. Furthermore, PPC being Big Endian means that the first 4 bytes will be 0 for any number lower than 2147483648.

[Thomas Voß (thomas-voss)]

LGTM.

review: Approve

[Michi Henning (michihenning)]

It would be ultra-cool to have a timeout test in the test suite.

[Michi Henning (michihenning)]

I finally got net-cpp to work with Thomas's help. Issue is that net-cpp needs libcurl4-openssl-dev.

We really need to add that to debian/control. Not sure about the other ssl options that libcurl supports. There is a whole bunch of them, including OpenSSL and gnutls.

Anyway, with the patched net-cpp, the timeout exception in the HttpClient test now is delivered correctly, and the test passes.

[Michi Henning (michihenning)]

I'm having second thoughts about this fix as it stands. If someone accidentally passes a duration that is greater than numeric_limits<long>::max() (such as the epoch), we'll wait a random amount of time (possibly not at all or a very short amount of time), depending on the exact value.
I can see three options:

- Check if the passed value overflows and silently clamp to numeric_limits<long>::max()

- Check if the passed value overflows and silently adjust to "wait forever"

- Throw an exception

I'm slightly in favour of the second option. That's because the max value amounts to a wait time of a little over 24 days. If someone is prepared to wait that long for an HTTP request to complete, it is probably safe to assume that they are willing to wait forever.

review: Needs Fixing

[Michi Henning (michihenning)]

I'm having second thoughts about this fix as it stands. If someone accidentally passes a duration that is greater than numeric_limits<long>::max() (such as the epoch), we'll wait a random amount of time (possibly not at all or a very short amount of time), depending on the exact value.
I can see three options:

- Check if the passed value overflows and silently clamp to numeric_limits<long>::max()

- Check if the passed value overflows and silently adjust to "wait forever"

- Throw an exception

I'm slightly in favour of the second option. That's because the max value amounts to a wait time of a little over 24 days. If someone is prepared to wait that long for an HTTP request to complete, it is probably safe to assume that they are willing to wait forever.

Also, a comment mentioning that the cast is necessary because set_option() uses varargs would be good. (It's not obvious, unless someone studies the curl doc rather carefully.)

review: Needs Fixing

[Michi Henning (michihenning)]

Here is a diff that does this:

=== modified file 'src/core/net/http/impl/curl/request.h'
--- src/core/net/http/impl/curl/request.h 2014-11-13 13:16:42 +0000
+++ src/core/net/http/impl/curl/request.h 2015-01-23 05:16:35 +0000
@@ -109,7 +109,11 @@
         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::timeout_ms, timeout.count());
+ // timeout.count() is a long long, but curl uses varargs and wants a long.
+ // If timeout.count() overflows a long, we wait forever instead of roughly 24.8 days.
+ auto count = timeout.count();
+ long adjusted_timeout = count <= std::numeric_limits<long>::max() ? count : 0;
+ easy.set_option(::curl::Option::timeout_ms, adjusted_timeout);
     }

     Response execute(const Request::ProgressHandler& ph)

[lp:~marcustomlinson/net-cpp/fix_ppc_timeout]

45. By Marcus Tomlinson on 2015-01-23

Use std::numeric_limits<long>::max() to ensure that our cast from long long to long does not exceed the upper limit of long.

[Marcus Tomlinson (marcustomlinson)]

> Here is a diff that does this:
>
> === modified file 'src/core/net/http/impl/curl/request.h'
> --- src/core/net/http/impl/curl/request.h 2014-11-13 13:16:42 +0000
> +++ src/core/net/http/impl/curl/request.h 2015-01-23 05:16:35 +0000
> @@ -109,7 +109,11 @@
> 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::timeout_ms, timeout.count());
> + // timeout.count() is a long long, but curl uses varargs and wants a
> long.
> + // If timeout.count() overflows a long, we wait forever instead of
> roughly 24.8 days.
> + auto count = timeout.count();
> + long adjusted_timeout = count <= std::numeric_limits<long>::max() ?
> count : 0;
> + easy.set_option(::curl::Option::timeout_ms, adjusted_timeout);
> }
>
> Response execute(const Request::ProgressHandler& ph)

Awesome, thanks for that. Done.

[Michi Henning (michihenning)]

LGTM :-)

review: Approve

=== modified file 'src/core/net/http/impl/curl/request.h'
--- src/core/net/http/impl/curl/request.h	2014-11-13 13:16:42 +0000
+++ src/core/net/http/impl/curl/request.h	2015-01-23 06:23:52 +0000
@@ -109,7 +109,11 @@
         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::timeout_ms, timeout.count());
+        // timeout.count() is a long long, but curl uses varargs and wants a long.
+        // If timeout.count() overflows a long, we wait forever instead of roughly 24.8 days.
+        auto count = timeout.count();
+        long adjusted_timeout = count <= std::numeric_limits<long>::max() ? count : 0;
+        easy.set_option(::curl::Option::timeout_ms, adjusted_timeout);
     }
 
     Response execute(const Request::ProgressHandler& ph)