Assertion fails if cancellation is requested before connection establish about redis HOT 12 CLOSED

I have created a new issue to improve cancellation support, see #32 so I will continue there. AFAICS, this ticket can be closed as the crashes have been fixed.

from redis.

You can easily reproduce it with simple loop like:

for (;;)
{
    auto timer = asio::steady_timer{ executor, std::chrono::milliseconds{ 10 } };
    co_await (connection.async_run(endpoint, asio::use_awaitable) || timer.async_wait(asio::use_awaitable));
}

from redis.

Thanks for reporting. Aedis was designed to provide built-in timeouts. The call to

co_await (connection.async_run(endpoint, asio::use_awaitable) || timer.async_wait(asio::use_awaitable));

is probably messing around with the internal operations in a way I did not test before. I am considering disabling this type of cancellation unless there is a reason for not doing so. I want to avoid every user implementing timeouts on their side over and over again. For all possible timeouts see https://github.com/mzimbres/aedis/blob/fd82204ba9e6e85b1011c96ebff79539c4227ddf/include/aedis/connection.hpp#L46

from redis.

Timeout here is just for demonstration, I'm not using timers like that I just cancel the connection for other reasons.
Point is that async_run is not can cancellable properly.

from redis.

How are you cancelling, with connection::cancel(operation::run)? Do you observe the same behaviour if you cancel like this

timer.async_wait([&](auto){conn.cancel(operation::run)})

I will investigate this issue on the weekend.

from redis.

Haven't tested that, but because I am using it in coroutines, it needs to be cancellable because even if I use current coroutines cancellation slot it will get overwritten by co_await conn.async_run(...)

from redis.

I can't reproduce the crash with this

#include <chrono>
#include <iostream>

#include <boost/asio.hpp>
#include <boost/asio/experimental/awaitable_operators.hpp>
#include <aedis.hpp>
#include <aedis/src.hpp>

namespace net = boost::asio;
using namespace net::experimental::awaitable_operators;
using aedis::endpoint;
using connection = aedis::connection<>;

net::awaitable<void> reconnect(std::shared_ptr<connection> db)
{
   net::steady_timer timer{co_await net::this_coro::executor};
   for (;;) {
      timer.expires_after(std::chrono::milliseconds{10});
      endpoint ep{"127.0.0.1", "6379"};
      co_await (
         db->async_run(ep, net::use_awaitable) ||
         timer.async_wait(net::use_awaitable)
      );
      std::cout << "Retrying" << std::endl;
   }
}

int main()
{
   net::io_context ioc;
   auto db = std::make_shared<connection>(ioc);
   net::co_spawn(ioc, reconnect(db), net::detached);
   ioc.run();
}

from redis.

The crash on line : ~/third_party/aedis/include/aedis/detail/connection_ops.hpp:536 is not possible on a normal cancellation. I am afraid this is indicating a buffer overflow somewhere in your program. Can you please enable sanitizers? For example, I get a crash if I use

      co_await (
         db->async_run({"127.0.0.1", "6379"}, net::use_awaitable) ||
         timer.async_wait(net::use_awaitable)
      );

instead of

      endpoint ep{"127.0.0.1", "6379"};
      co_await (
         db->async_run(ep, net::use_awaitable) ||
         timer.async_wait(net::use_awaitable)
      );

from redis.

I can't reproduce the crash with this

#include <chrono>
#include <iostream>

#include <boost/asio.hpp>
#include <boost/asio/experimental/awaitable_operators.hpp>
#include <aedis.hpp>
#include <aedis/src.hpp>

namespace net = boost::asio;
using namespace net::experimental::awaitable_operators;
using aedis::endpoint;
using connection = aedis::connection<>;

net::awaitable<void> reconnect(std::shared_ptr<connection> db)
{
   net::steady_timer timer{co_await net::this_coro::executor};
   for (;;) {
      timer.expires_after(std::chrono::milliseconds{10});
      endpoint ep{"127.0.0.1", "6379"};
      co_await (
         db->async_run(ep, net::use_awaitable) ||
         timer.async_wait(net::use_awaitable)
      );
      std::cout << "Retrying" << std::endl;
   }
}

int main()
{
   net::io_context ioc;
   auto db = std::make_shared<connection>(ioc);
   net::co_spawn(ioc, reconnect(db), net::detached);
   ioc.run();
}

I can reproduce it with your code here, it takes a little time to happen (most of the times under 60 secs). some times it leads to that assertion fail some times dead lock.
Try change the time form 10ms to 6ms it happens faster.
And of course make sure you are in debug build.
Changing timer to something low like 1ms leads to a new assertion fail:
test: ~/third_party/aedis/include/aedis/detail/net.hpp:101: void aedis::detail::resolve_op<boost::asio::ip::basic_resolver<boost::asio::ip::tcp>, boost::asio::basic_waitable_timer<std::chrono::steady_clock>>::operator()(Self &, std::array<std::size_t, 2>, boost::system::error_code, boost::asio::ip::tcp::resolver::results_type, boost::system::error_code) [Resolver = boost::asio::ip::basic_resolver<boost::asio::ip::tcp>, Timer = boost::asio::basic_waitable_timer<std::chrono::steady_clock>, Self = boost::asio::detail::composed_op<aedis::detail::resolve_op<boost::asio::ip::basic_resolver<boost::asio::ip::tcp>, boost::asio::basic_waitable_timer<std::chrono::steady_clock>>, boost::asio::detail::composed_work<void (boost::asio::any_io_executor, boost::asio::any_io_executor)>, boost::asio::detail::composed_op<aedis::detail::resolve_with_timeout_op<aedis::connection<>>, boost::asio::detail::composed_work<void (boost::asio::any_io_executor)>, boost::asio::detail::composed_op<aedis::detail::run_op<aedis::connection<>>, boost::asio::detail::composed_work<void (boost::asio::any_io_executor)>, boost::asio::detail::awaitable_handler<boost::asio::any_io_executor, boost::system::error_code>, void (boost::system::error_code)>, void (boost::system::error_code)>, void (boost::system::error_code, boost::asio::ip::basic_resolver_results<boost::asio::ip::tcp>)>]: Assertion (!ec2)&&("resolve_op: Incompatible state.")' failed.

from redis.

The second assertion you found is something I thought was impossible to happen but does not represent a pre condition for my code to work. I will remove it since it can indeed happen. Thanks again for reporting.

from redis.

This commit 3a03a43 fixes the problems I could reproduce so far. There is still a problem though if use very short timeouts like 1ms, in this case it seems that the cancellation gets lost. I will have to investigate more.

from redis.

Are you using the same commit that I mentioned in first comment?
here is another assertion fail:
test: ~/third_party/aedis/include/aedis/resp3/detail/exec.hpp:133: void aedis::resp3::detail::exec_with_timeout_op<boost::asio::basic_stream_socket<boost::asio::ip::tcp>, boost::asio::basic_waitable_timer<std::chrono::steady_clock>, aedis::adapter::detail::general_aggregate<std::vector<aedis::resp3::node<std::string>>>, boost::asio::dynamic_string_buffer<char, std::char_traits<char>, std::allocator<char>>>::operator()(Self &, std::array<std::size_t, 2>, boost::system::error_code, std::size_t, boost::system::error_code) [AsyncStream = boost::asio::basic_stream_socket<boost::asio::ip::tcp>, Timer = boost::asio::basic_waitable_timer<std::chrono::steady_clock>, Adapter = aedis::adapter::detail::general_aggregate<std::vector<aedis::resp3::node<std::string>>>, DynamicBuffer = boost::asio::dynamic_string_buffer<char, std::char_traits<char>, std::allocator<char>>, Self = boost::asio::detail::composed_op<aedis::resp3::detail::exec_with_timeout_op<boost::asio::basic_stream_socket<boost::asio::ip::tcp>, boost::asio::basic_waitable_timer<std::chrono::steady_clock>, aedis::adapter::detail::general_aggregate<std::vector<aedis::resp3::node<std::string>>>, boost::asio::dynamic_string_buffer<char, std::char_traits<char>, std::allocator<char>>>, boost::asio::detail::composed_work<void (boost::asio::any_io_executor, boost::asio::any_io_executor)>, boost::asio::detail::composed_op<aedis::detail::run_op<aedis::connection<>>, boost::asio::detail::composed_work<void (boost::asio::any_io_executor)>, boost::asio::detail::awaitable_handler<boost::asio::any_io_executor, boost::system::error_code>, void (boost::system::error_code)>, void (boost::system::error_code, unsigned long)>]: Assertion (!ec2)&&("exec_with_timeout_op: Unexpected completion order.")' failed.

Maybe using the following code helps to reproduce it faster:

#include <boost/asio.hpp>
#include <boost/asio/experimental/awaitable_operators.hpp>

#include <aedis.hpp>
#include <aedis/src.hpp>
#include <chrono>
#include <iostream>

namespace net = boost::asio;
using namespace net::experimental::awaitable_operators;
using aedis::endpoint;
using connection = aedis::connection<>;

net::awaitable< void >
reconnect()
{
    auto executor = co_await net::this_coro::executor;
    auto db1      = connection{ executor };
    auto db2      = connection{ executor };
    auto db3      = connection{ executor };
    auto db4      = connection{ executor };
    auto timer    = net::steady_timer{ executor };
    for (int i = 0;; i++)
    {
        timer.expires_after(std::chrono::milliseconds{ 10 });
        endpoint ep{ "127.0.0.1", "6379" };
        co_await (db1.async_run(ep, net::use_awaitable) || db2.async_run(ep, net::use_awaitable) ||
                  db3.async_run(ep, net::use_awaitable) || db4.async_run(ep, net::use_awaitable) ||
                  timer.async_wait(net::use_awaitable));
        std::cout << i << std::endl;
    }
}

int
main()
{
    net::io_context ioc;
    net::co_spawn(ioc, reconnect(), net::detached);
    ioc.run();
}

from redis.