Error fixes and test coverage improvements.

CHANGELOG.md

@@ -7,6 +7,15 @@
 
 * Replaces autotools with CMake.
 
+* Fixes a bug in `aedis::adapt()` that would cause RESP3 errors to be
+  ignored. One consequence of that behaviour is that
+  `connection::async_run` would not exit with failure in servers that
+  required authentication.
+
+* Fixes a bug in `connection::async_run` that would cause it to
+  complete with success when an error in the `connection::async_exec`
+  occurred.
+
 ## v1.0.0
 
 * Adds experimental cmake support for windows users.
@@ -31,7 +40,7 @@
   complete under certain conditions.
 
 * Bugfix: Documentation of `adapt()` functions were missing from
-  doxygen.
+  Doxygen.
 
 ## v0.3.0
 
@@ -85,7 +94,7 @@
 ## v0.2.0
 
 * Major rewrite of the high-level API. There is no more need to use the low-level API anymore.
-* No more callbacks: Sending requests follows the ASIO asynchrnous model.
+* No more callbacks: Sending requests follows the ASIO asynchronous model.
 * Support for reconnection: Pending requests are not canceled when a connection is lost and are re-sent when a new one is established.
 * The library is not sending HELLO-3 on user behalf anymore. This is important to support AUTH properly.
 

CMakeLists.txt

@@ -39,6 +39,7 @@ add_executable(subscriber_sync examples/subscriber_sync.cpp)
 add_executable(test_low_level tests/low_level.cpp)
 add_executable(test_connection tests/connection.cpp)
 add_executable(test_connection_push tests/connection_push.cpp)
+add_executable(test_connection_quit tests/connection_quit.cpp)
 add_executable(low_level_sync tests/low_level_sync.cpp)
 
 # Tests
@@ -49,7 +50,9 @@ add_test(intro intro)
 add_test(intro_sync intro_sync)
 add_test(serialization serialization)
 add_test(test_low_level test_low_level)
+add_test(test_connection test_connection)
 add_test(test_connection_push test_connection_push)
+add_test(test_connection_quit test_connection_quit)
 add_test(low_level_sync low_level_sync)
 
 # Install

include/aedis/connection.hpp

@@ -215,9 +215,8 @@ public:
             push_channel_.cancel();
             return 1U;
          }
+         default: BOOST_ASSERT(false); return 0;
       }
-
-      return 0;
    }
 
    /// Get the config object.

include/aedis/detail/connection_ops.hpp

@@ -431,8 +431,10 @@ struct run_one_op {
          }
 
          conn->req_.clear();
-         if (!std::empty(conn->cfg_.username) && !std::empty(conn->cfg_.password))
+         if (!std::empty(conn->cfg_.username) && !std::empty(conn->cfg_.password)) {
             conn->req_.push("AUTH", conn->cfg_.username, conn->cfg_.password);
+         }
+
          conn->req_.push("HELLO", "3");
 
          conn->ping_timer_.expires_after(conn->cfg_.ping_interval);
@@ -618,7 +620,7 @@ struct reader_op {
             yield
             conn->read_timer_.async_wait(std::move(self));
             if (!conn->socket_->is_open()) {
-               self.complete({});
+               self.complete(ec);
                return;
             }
          }

include/aedis/error.hpp

@@ -34,9 +34,6 @@ enum class error
    /// Can't parse the string as a number.
    not_a_number,
 
-   /// Received less bytes than expected.
-   unexpected_read_size,
-
    /// The maximum depth of a nested response was exceeded.
    exceeeds_max_nested_depth,
 

include/aedis/impl/error.ipp

@@ -25,7 +25,6 @@ struct error_category_impl : boost::system::error_category {
 	 case error::exec_timeout: return "Exec timeout.";
 	 case error::invalid_data_type: return "Invalid resp3 type.";
 	 case error::not_a_number: return "Can't convert string to number.";
-	 case error::unexpected_read_size: return "Unexpected read size.";
 	 case error::exceeeds_max_nested_depth: return "Exceeds the maximum number of nested responses.";
 	 case error::unexpected_bool_value: return "Unexpected bool value.";
 	 case error::empty_field: return "Expected field value is empty.";
@@ -39,9 +38,7 @@ struct error_category_impl : boost::system::error_category {
 	 case error::incompatible_size: return "Aggregate container has incompatible size.";
 	 case error::not_a_double: return "Not a double.";
 	 case error::resp3_null: return "Got RESP3 null.";
-	 default:
-            BOOST_ASSERT(false);
-            return "Aedis error.";
+	 default: BOOST_ASSERT(false); return "Aedis error.";
       }
    }
 };

include/aedis/resp3/detail/read_ops.hpp

@@ -23,7 +23,14 @@ namespace detail {
 #include <boost/asio/yield.hpp>
 
 struct ignore_response {
-   void operator()(node<boost::string_view>, boost::system::error_code&) { }
+   void operator()(node<boost::string_view> nd, boost::system::error_code& ec)
+   {
+      switch (nd.data_type) {
+         case resp3::type::simple_error: ec = error::resp3_simple_error; return;
+         case resp3::type::blob_error: ec = error::resp3_blob_error; return;
+         default: return;
+      }
+   }
 };
 
 template <
@@ -61,6 +68,7 @@ public:
                self.complete(ec, 0);
                return;
             }
+
          } else {
 	    // On a bulk read we can't read until delimiter since the
 	    // payload may contain the delimiter itself so we have to

include/aedis/resp3/read.hpp

@@ -70,10 +70,6 @@ read(
 	 if (ec)
 	    return 0;
 
-	 if (n < 3) {
-            ec = error::unexpected_read_size;
-            return 0;
-         }
       } else {
 	 auto const s = buf.size();
 	 auto const l = p.bulk_length();
@@ -83,11 +79,6 @@ read(
 	    n = boost::asio::read(stream, buf.data(s, to_read), ec);
 	    if (ec)
 	       return 0;
-
-            if (n < to_read) {
-               ec = error::unexpected_read_size;
-               return 0;
-            }
 	 }
       }
 

tests/connection.cpp

@@ -40,6 +40,7 @@ bool is_host_not_found(boost::system::error_code ec)
 // Tests whether resolve fails with the correct error.
 BOOST_AUTO_TEST_CASE(test_resolve)
 {
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
    connection::config cfg;
    cfg.host = "Atibaia";
    cfg.port = "6379";
@@ -58,6 +59,7 @@ BOOST_AUTO_TEST_CASE(test_resolve)
 
 BOOST_AUTO_TEST_CASE(test_resolve_with_timeout)
 {
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
    net::io_context ioc;
 
    connection db{ioc};
@@ -77,6 +79,7 @@ BOOST_AUTO_TEST_CASE(test_resolve_with_timeout)
 
 BOOST_AUTO_TEST_CASE(test_connect)
 {
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
    connection::config cfg;
    cfg.host = "127.0.0.1";
    cfg.port = "1";
@@ -94,6 +97,7 @@ BOOST_AUTO_TEST_CASE(test_connect)
 
 BOOST_AUTO_TEST_CASE(test_connect_timeout)
 {
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
    net::io_context ioc;
    connection db{ioc};
    db.get_config().host = "example.com";
@@ -106,60 +110,6 @@ BOOST_AUTO_TEST_CASE(test_connect_timeout)
    ioc.run();
 }
 
-//----------------------------------------------------------------
-
-// Test if quit causes async_run to exit.
-void test_quit1(connection::config const& cfg)
-{
-   net::io_context ioc;
-   auto db = std::make_shared<connection>(ioc, cfg);
-
-   request req;
-   req.push("QUIT");
-
-   db->async_exec(req, adapt(), [](auto ec, auto){
-      BOOST_TEST(!ec);
-   });
-
-   db->async_run([](auto ec){
-      BOOST_CHECK_EQUAL(ec, net::error::misc_errors::eof);
-   });
-
-   ioc.run();
-}
-
-void test_quit2(connection::config const& cfg)
-{
-   std::cout << "test_quit2" << std::endl;
-   request req;
-   req.push("QUIT");
-
-   net::io_context ioc;
-   auto db = std::make_shared<connection>(ioc, cfg);
-   db->async_run(req, adapt(), [](auto ec, auto){
-      BOOST_TEST(!ec);
-   });
-
-   ioc.run();
-}
-
-BOOST_AUTO_TEST_CASE(test_quit)
-{
-   connection::config cfg;
-
-   cfg.coalesce_requests = true;
-   test_quit1(cfg);
-
-   cfg.coalesce_requests = false;
-   test_quit1(cfg);
-
-   cfg.coalesce_requests = true;
-   test_quit2(cfg);
-
-   cfg.coalesce_requests = false;
-   test_quit2(cfg);
-}
-
 #ifdef BOOST_ASIO_HAS_CO_AWAIT
 
 net::awaitable<void> send_after(std::shared_ptr<connection> db, std::chrono::milliseconds ms)
@@ -177,6 +127,7 @@ net::awaitable<void> send_after(std::shared_ptr<connection> db, std::chrono::mil
 
 BOOST_AUTO_TEST_CASE(test_idle)
 {
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
    std::chrono::milliseconds ms{5000};
 
    {
@@ -257,6 +208,7 @@ net::awaitable<void> test_reconnect_impl(std::shared_ptr<connection> db)
 // Test whether the client works after a reconnect.
 BOOST_AUTO_TEST_CASE(test_reconnect)
 {
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
    net::io_context ioc;
    auto db = std::make_shared<connection>(ioc.get_executor());
    db->get_config().enable_events = true;
@@ -274,6 +226,7 @@ BOOST_AUTO_TEST_CASE(test_reconnect)
 
 BOOST_AUTO_TEST_CASE(test_auth_fail)
 {
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
    net::io_context ioc;
    auto db = std::make_shared<connection>(ioc.get_executor());
 

tests/connection_quit.cpp

@@ -0,0 +1,122 @@
+/* Copyright (c) 2018-2022 Marcelo Zimbres Silva (mzimbres@gmail.com)
+ *
+ * Distributed under the Boost Software License, Version 1.0. (See
+ * accompanying file LICENSE.txt)
+ */
+
+#include <iostream>
+#include <boost/asio.hpp>
+#include <boost/system/errc.hpp>
+#include <boost/asio/experimental/as_tuple.hpp>
+
+#define BOOST_TEST_MODULE low level
+#include <boost/test/included/unit_test.hpp>
+
+#include <aedis.hpp>
+#include <aedis/src.hpp>
+
+namespace net = boost::asio;
+
+using aedis::resp3::request;
+using aedis::adapt;
+using connection = aedis::connection<>;
+using error_code = boost::system::error_code;
+using net::experimental::as_tuple;
+
+bool is_host_not_found(boost::system::error_code ec)
+{
+   if (ec == net::error::netdb_errors::host_not_found) return true;
+   if (ec == net::error::netdb_errors::host_not_found_try_again) return true;
+   return false;
+}
+
+// Test if quit causes async_run to exit.
+BOOST_AUTO_TEST_CASE(test_quit_no_coalesce)
+{
+   net::io_context ioc;
+   auto db = std::make_shared<connection>(ioc);
+   db->get_config().coalesce_requests = false;
+
+   request req1;
+   req1.push("PING");
+
+   request req2;
+   req2.push("QUIT");
+
+   db->async_exec(req1, adapt(), [](auto ec, auto){ BOOST_TEST(!ec); });
+   db->async_exec(req2, adapt(), [](auto ec, auto){ BOOST_TEST(!ec); });
+   db->async_exec(req1, adapt(), [](auto ec, auto){
+      BOOST_CHECK_EQUAL(ec, boost::system::errc::errc_t::operation_canceled);
+   });
+   db->async_exec(req1, adapt(), [](auto ec, auto){
+         BOOST_CHECK_EQUAL(ec, boost::system::errc::errc_t::operation_canceled);
+   });
+   db->async_exec(req1, adapt(), [](auto ec, auto){
+      BOOST_CHECK_EQUAL(ec, boost::system::errc::errc_t::operation_canceled);
+   });
+
+   db->async_run([db](auto ec){
+      BOOST_CHECK_EQUAL(ec, net::error::misc_errors::eof);
+      db->cancel(connection::operation::exec);
+   });
+
+   ioc.run();
+}
+
+BOOST_AUTO_TEST_CASE(test_quit_coalesce)
+{
+   net::io_context ioc;
+   auto db = std::make_shared<connection>(ioc);
+
+   request req1;
+   req1.push("PING");
+
+   request req2;
+   req2.push("QUIT");
+
+   db->async_exec(req1, adapt(), [](auto ec, auto){
+      BOOST_TEST(!ec);
+   });
+   db->async_exec(req2, adapt(), [](auto ec, auto){
+      BOOST_TEST(!ec);
+   });
+   db->async_exec(req1, adapt(), [](auto ec, auto){
+      BOOST_CHECK_EQUAL(ec, net::error::misc_errors::eof);
+   });
+   db->async_exec(req1, adapt(), [](auto ec, auto){
+      BOOST_CHECK_EQUAL(ec, boost::system::errc::errc_t::operation_canceled);
+   });
+
+   db->async_run([db](auto ec){
+      BOOST_CHECK_EQUAL(ec, boost::system::errc::errc_t::operation_canceled);
+      db->cancel(connection::operation::exec);
+   });
+
+   ioc.run();
+}
+
+void test_quit2(connection::config const& cfg)
+{
+   std::cout << "test_quit2" << std::endl;
+   request req;
+   req.push("QUIT");
+
+   net::io_context ioc;
+   auto db = std::make_shared<connection>(ioc, cfg);
+   db->async_run(req, adapt(), [](auto ec, auto){ BOOST_TEST(!ec); });
+
+   ioc.run();
+}
+
+BOOST_AUTO_TEST_CASE(test_quit)
+{
+   std::cout << boost::unit_test::framework::current_test_case().p_name << std::endl;
+   connection::config cfg;
+
+   cfg.coalesce_requests = true;
+   test_quit2(cfg);
+
+   cfg.coalesce_requests = false;
+   test_quit2(cfg);
+}
+

tests/low_level.cpp

@@ -28,6 +28,7 @@ namespace resp3 = aedis::resp3;
 using test_stream = boost::beast::test::stream;
 using aedis::adapter::adapt2;
 using node_type = aedis::resp3::node<std::string>;
+using vec_node_type = std::vector<node_type>;
 
 //-------------------------------------------------------------------
 
@@ -145,6 +146,7 @@ std::vector<node_type> streamed_string_e2 { {resp3::type::streamed_string_part,
    test(ex, make_expected(":3\r\n",        node_type{resp3::type::number, 1UL, 0UL, {"3"}}, "number.node (positive)")); \
    test(ex, make_expected("_\r\n",         int{0}, "number.int.error.null", aedis::error::resp3_null)); \
    test(ex, make_expected(streamed_string_wire, std::string{"Hello word"}, "streamed_string.string")); \
+   test(ex, make_expected(streamed_string_wire, int{}, "streamed_string.string", aedis::error::not_a_number)); \
    test(ex, make_expected(streamed_string_wire, streamed_string_e1, "streamed_string.node")); \
 
 BOOST_AUTO_TEST_CASE(test_push)
@@ -334,6 +336,7 @@ BOOST_AUTO_TEST_CASE(test_array)
    net::io_context ioc;
    char const* wire = "*3\r\n$2\r\n11\r\n$2\r\n22\r\n$1\r\n3\r\n";
    char const* wire_nested = "*1\r\n*1\r\n$2\r\nab\r\n";
+   char const* wire_nested2 = "*1\r\n*1\r\n*1\r\n*1\r\n*1\r\n*1\r\na\r\n";
 
    std::vector<node_type> e1a
       { {resp3::type::array,       3UL, 0UL, {}}
@@ -365,6 +368,7 @@ BOOST_AUTO_TEST_CASE(test_array)
    auto const in13 = expect<array_type2>{wire_nested, array_type2{}, "array.nested", aedis::error::nested_aggregate_not_supported};
    auto const in14 = expect<array_type2>{wire, array_type2{}, "array.null", aedis::error::incompatible_size};
    auto const in15 = expect<array_type2>{":3\r\n", array_type2{}, "array.array", aedis::error::expects_resp3_aggregate};
+   auto const in16 = expect<vec_node_type>{wire_nested2, vec_node_type{}, "array.depth.exceeds", aedis::error::exceeeds_max_nested_depth};
 
    auto ex = ioc.get_executor();
 
@@ -383,6 +387,7 @@ BOOST_AUTO_TEST_CASE(test_array)
    test_sync(ex, in13);
    test_sync(ex, in14);
    test_sync(ex, in15);
+   test_sync(ex, in16);
 
    test_async(ex, in01);
    test_async(ex, in02);
@@ -399,6 +404,7 @@ BOOST_AUTO_TEST_CASE(test_array)
    test_async(ex, in13);
    test_async(ex, in14);
    test_async(ex, in15);
+   test_async(ex, in16);
 
    ioc.run();
 }
@@ -654,6 +660,7 @@ BOOST_AUTO_TEST_CASE(test_resp3)
    test_async(ex, in06);
    test_async(ex, in07);
    test_async(ex, in08);
+
    ioc.run();
 }
 
@@ -745,7 +752,6 @@ BOOST_AUTO_TEST_CASE(error)
    check_error("aedis", aedis::error::exec_timeout);
    check_error("aedis", aedis::error::invalid_data_type);
    check_error("aedis", aedis::error::not_a_number);
-   check_error("aedis", aedis::error::unexpected_read_size);
    check_error("aedis", aedis::error::exceeeds_max_nested_depth);
    check_error("aedis", aedis::error::unexpected_bool_value);
    check_error("aedis", aedis::error::empty_field);
@@ -814,3 +820,4 @@ BOOST_AUTO_TEST_CASE(type_convert)
    CHECK_CASE(streamed_string_part);
 #undef CHECK_CASE
 }
+