Skip to content

Add parse event init, node and done. #286

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

mzimbres
Copy link
Collaborator

@mzimbres mzimbres commented Jul 25, 2025

This PR implements parse events and is a prerequisite to the generic_flat_response from @D0zee.

@mzimbres mzimbres requested a review from anarthal July 25, 2025 07:49
* @endcode
*/
class any_adapter {
using any_adapter = std::function<void(parse_event, resp3::node_view const&, system::error_code&)>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is the right interface for any_adapter, because

  • There's now a discrepancy between what adapters support (3 functions with different parameters) and what any_adapter is (a single function with an enum)
  • Users can't create any_adapter objects, since the only practical way is using make_any_adapter, which is in detail.

Instead of this, I'd make the std::function and the enum an implementation detail, like this:

class any_adapter {
   enum class parse_event
   {
      /// Called before the parser starts processing data
      init,
      /// Called for each and every node of RESP3 data
      node,
      /// Called when done processing a complete RESP3 message
      done
   };

   using fn_type = std::function<void(parse_event, resp3::node_view const*, system::error_code*)>;
   fn_type impl_;

   template <class Response>
   static fn_type create_impl(Response& r)
   {
      using namespace boost::redis::adapter;

      return [adapter = boost_redis_adapt(
                 r)](parse_event ev, resp3::node_view const* nd, system::error_code* ec) mutable {
         switch (ev) {
            case parse_event::init: adapter.on_init(); break;
            case parse_event::node: adapter.on_node(*nd, *ec); break;
            case parse_event::done: adapter.on_done(); break;
         }
      };
   }

public:
   template <class T, class = std::enable_if_t<!std::is_same_v<T, any_adapter>>>
   explicit any_adapter(T& resp)
   : impl_(create_impl(resp))
   { }

   void on_init() { impl_(parse_event::init, nullptr, nullptr); };

   void on_done() { impl_(parse_event::done, nullptr, nullptr); };

   void on_node(resp3::node_view const& nd, system::error_code& ec)
   {
      impl_(parse_event::node, &nd, &ec);
   };
};


private:
any_adapter adapter_;
std::size_t expected_responses_ = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this belongs here. You already maintain this information in multiplexer::elem::remaining_responses_. You can use this variable and go.

BOOST_CHECK_NO_THROW(any_adapter{r2});
BOOST_CHECK_NO_THROW(any_adapter{r3});
BOOST_CHECK_NO_THROW(any_adapter{ignore});
BOOST_CHECK_NO_THROW(make_any_adapter(r1));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that these should work as they are (without the make_).

@mzimbres
Copy link
Collaborator Author

@anarthal I think all concerns were addressed, please have a look at it again.

using fn_type = std::function<void(std::size_t, resp3::node_view const&, system::error_code&)>;
/** @brief Parse events that an adapter must support.
*/
enum class parse_event
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the point of making parse_event, impl_type and the constructor from impl_type public and the constructor from the adapter private? parse_event and impl_type look like private things, while as a user, I want to be able to construct any_adapter objects from typed adapters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the Boost review customization for your own response was to based on providing boost_redis_adapt() that would be found via ADL. No async_exec overload that took adapters were expose.

Now that adapters are exposed in the async_exec overload it does not make sense to provide boost_redis_adapt for a custom response since the user can just construct an any_adapter himself by passing an impl_type. To enable that we have to make impl_type and parse_event public types.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't the user just write an adapter like this?

class my_adapter {
  // impl
public:
    void on_init() { /* */ }
    void on_node(resp3::node_view const&, system::error_code&) { /* */ }
    void on_finish {}
};

// Now use it
conn.async_exec(req, any_adapter(my_adapter(/* whatever */));

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is supported in this PR:

template <class Adapter>
any_adapter(Adapter adapter)
{
impl_ = [adapter2 = std::move(adapter)](
any_adapter::parse_event ev,
resp3::node_view const& nd,
system::error_code& ec) mutable {
switch (ev) {
case parse_event::init: adapter2.on_init(); break;
case parse_event::node: adapter2.on_node(nd, ec); break;
case parse_event::done: adapter2.on_done(); break;
}
};
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It says "it is only used internally, so not documented"

static auto create_impl(T& resp) -> impl_t
// No need to document this as it is only used internally.
template <class Adapter>
any_adapter(Adapter adapter)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this an Adapter (vs a Response&) is a breaking API change. To be honest, it makes sense for any_adapter to be constructible from an Adapter rather than a Response&, though. It is also true that all uses I've found end up calling boost_redis_adapt, so this feels like making syntax a little bit more convoluted. Is there any chance to make any_adapter(response) keep working?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought this was a small enough breaking change that wouldn't hurt anyone.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will break at least the user who originally requested any_adapter

@anarthal
Copy link
Collaborator

@mzimbres see my comments. I still have concerns about any_adapter's current interface.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants