Request must outlive async_exec?

[bruno-viva]

First of all, thanks for this amazing library! It makes it very easy to integrate with Redis. Now onto the issue:

When calling async_exec, it is taking the request and keeping a pointer to it (in req_info). So a code like this:

void SetKey(std::string_view key, std::string_view value, boost::redis::connection& connection) {
  boost::redis::request request;
  request.push("SET", key, value);
  auto response = std::make_unique<boost::redis::response<std::string>>();
  auto& response_ref = *response;
  connection.async_exec(
      request, response_ref,
      [response = std::move(response)](const boost::system::error_code& error, std::size_t) {
        if (error) {
          std::cout << "Failed to send SET command to Redis server: " << error.what() << std::endl;
        } else {
          std::cout << "Response: " << std::get<0>(*response).value() << std::endl;
        }
      });
  // **Boom, request goes out of scope.**
}

Is invalid and has undefined behavior. I am using C++17, and so I prefer to not try the coroutines version.

I am not sure what the correct fix to the library is here (Should probably keep a copy of the request? Have const request& and request&& alternatives to copy/move the request in?). But I think the documentation could mention that, as it was not immediately obvious. In other words, the response is kind of obvious that needs to outlive it, but not the request! Some more C++17 examples would help too.

For completeness, here is a working version:

void SetKey(std::string_view key, std::string_view value, boost::redis::connection& connection) {
  auto request = std::make_unique<boost::redis::request>();
  request->push("SET", key, value);
  auto response = std::make_unique<boost::redis::response<std::string>>();
  auto& request_ref = *request;
  auto& response_ref = *response;
  connection.async_exec(
      request_ref, response_ref,
      [request = std::move(request), response = std::move(response)](const boost::system::error_code& error, std::size_t) {
        if (error) {
          std::cout << "Failed to send SET command to Redis server: " << error.what() << std::endl;
        } else {
          std::cout << "Response: " << std::get<0>(*response).value() << std::endl;
        }
      });
}

Thanks again for this lib! Looking forward to hearing your thoughts on this.

[mzimbres]

Hi Bruno. Yes, you have to guarantee that the request and response outlives the async operation, this is usually achieved with a shared_ptr, for example

auto request = std::make_shared<boost::redis::request>();
request->push("SET", key, value);
auto response = std::make_shared<boost::redis::response<std::string>>();

connection.async_exec(*request, *response, [request, response](auto const& error, std::size_t) {
     ...
});

Alternatively you can also have a look at the my cpp17_intro_sync.cpp example. The code above would look like this in this case

sync_connection conn;
conn.run(cfg);

request req;
request->push("SET", key, value);
response<std::string> resp;

conn.exec(req, resp);
conn.stop();

The sync connection type is not part of the library but provided as an example in sync_connection.hpp, which you can copy to your project.

[bruno-viva]

Thanks Marcelo! It makes sense now. Do you think the API docs should state that or maybe the API should be modified to make it clearer?

It isn't immediately obvious when reading the function definition:

auto async_exec(request const& req, Response& resp, CompletionToken token)

request seems cheap to move, and could be moved in to req_info, instead of it being a pointer. Was there a reason on why it was kept as a pointer? Thanks in advance

[anarthal]

Without knowing the actual reasons, I'd speculate that this pattern allows to reuse request objects, while moving them wouldn't.