Skip to content
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

Replace CAF deserializers with Broker decoders #424

Merged
merged 9 commits into from
Oct 19, 2024
Merged

Conversation

Neverlord
Copy link
Member

When implementing the new networking layer and broker::variant, we have implemented encoders for the binary serialization format. However, the decoding has only been implemented for broker::variant.

This set of changes implements the decoding as well, meaning that Broker is now fully "self-hosted" and no longer relies on the binary serialization primitives in CAF which make no guarantees on the stability of the format.

There are two "flavors" for the decoding:

  • A free decode function with a SAX-like interface. This function is tailored towards Broker's type system and can be used for decoding broker::variant and broker::data.
  • A decoder class that is designed to work with the inspect API. We use this as a drop-in replacement for caf::deserializer, e.g., when decoding store commands.

Closes #161.

@Neverlord
Copy link
Member Author

Neverlord commented Aug 6, 2024

@timwoj @awelzel ready to review, if one of you'd like to pick it up. The CI error on alpine is unrelated (Cirrus fails to pull the image, apparently).

@ckreibich ckreibich self-requested a review August 15, 2024 19:51
Copy link
Member

@ckreibich ckreibich left a comment

Choose a reason for hiding this comment

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

LGTM Dominik, thanks. I've run Zeek's testsuites with it and they ran fine. I tried to eyeball performance via broker-throughput and it runs fine on this branch, but for master the server is currently throwing lots of deserialization_failed("failed to parse data") errors — I tried

$  ./broker-throughput --verbose --server :8888

and

$  ./broker-throughput --verbose -t 3 -r 1000 localhost:8888

... I can't recall whether this is known — do you know? We'll see performance numbers also via Zeek's CI setup when we bump Broker.

I can merge this too, if you're out of time.

CHECK_EQ(cpy.sender, cmd.sender);
CHECK_EQ(cpy.receiver, cmd.receiver);
if (CHECK(std::holds_alternative<cumulative_ack_command>(cpy.content))) {
CHECK_EQ(std::get<cumulative_ack_command>(cpy.content).seq, 42);
Copy link
Member

Choose a reason for hiding this comment

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

Just a quick suggestion here since GCC 14.2 warns:

Suggested change
CHECK_EQ(std::get<cumulative_ack_command>(cpy.content).seq, 42);
CHECK_EQ(std::get<cumulative_ack_command>(cpy.content).seq, sequence_number_type{42});
In file included from /home/christian/devel/zeek/zeek/auxil/broker/caf/libcaf_test/caf/test/dsl.hpp:15,
                 from /home/christian/devel/zeek/zeek/auxil/broker/caf/libcaf_test/caf/test/bdd_dsl.hpp:3,
                 from /home/christian/devel/zeek/zeek/auxil/broker/libbroker/broker/broker-test.test.hh:7,
                 from /home/christian/devel/zeek/zeek/auxil/broker/libbroker/broker/format/bin.test.cc:3:
/home/christian/devel/zeek/zeek/auxil/broker/libbroker/broker/format/bin.test.cc: In instantiation of ‘{anonymous}::test396::run_test_impl()::<lambda(const auto:264&, const auto:265&)> [with auto:264 = long unsigned int; auto:265 = int]’:
/home/christian/devel/zeek/zeek/auxil/broker/libbroker/broker/format/bin.test.cc:434:5:   required from here
  596 |   [](const auto& x_val, const auto& y_val) {                                   \
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  597 |     return ::caf::test::detail::check_bin(                                     \
      |     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  598 |       x_val == y_val, __FILE__, __LINE__, #x_expr " == " #y_expr,              \
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  599 |       caf::detail::stringification_inspector::render(x_val),                   \
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  600 |       caf::detail::stringification_inspector::render(y_val));                  \
      |       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  601 |   }(x_expr, y_expr)
      |   ~^~~~~~~~~~~~~~~~
/home/christian/devel/zeek/zeek/auxil/broker/caf/libcaf_test/caf/test/unit_test.hpp:598:13: warning: comparison of integer expressions of different signedness: ‘const long unsigned int’ and ‘const int’ [-Wsign-compare]
  598 |       x_val == y_val, __FILE__, __LINE__, #x_expr " == " #y_expr,              \
      |       ~~~~~~^~~~~~~~
/home/christian/devel/zeek/zeek/auxil/broker/caf/libcaf_test/caf/test/bdd_dsl.hpp:41:28: note: in expansion of macro ‘CAF_CHECK_EQUAL’
   41 | #define CHECK_EQ(lhs, rhs) CAF_CHECK_EQUAL(lhs, rhs)
      |                            ^~~~~~~~~~~~~~~
/home/christian/devel/zeek/zeek/auxil/broker/libbroker/broker/format/bin.test.cc:434:5: note: in expansion of macro ‘CHECK_EQ’
  434 |     CHECK_EQ(std::get<cumulative_ack_command>(cpy.content).seq, 42);
      |     ^~~~~~~~

auto proto = uint8_t{0};
if (!read(pos, end, proto))
return {false, pos};
if (proto > 3) // 3 is the highest protocol number we support (ICMP).
Copy link
Member

Choose a reason for hiding this comment

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

Total nit, but if we know this is about ICMP we could just say broker::port::protocol::icmp.

@Neverlord
Copy link
Member Author

but for master the server is currently throwing lots of deserialization_failed("failed to parse data") errors — I tried

Hm, that news to me. I don't think I'll be able to dig into that today. I'd like to look into that before we merge the PR, even if it means letting this sit for a while until I'm back. Would you be OK with that?

@ckreibich
Copy link
Member

Yeah no problem, I doubt we'll make other changes that would invalidate your PR. Have a great trip!

@Neverlord
Copy link
Member Author

Ok, so there's been two things. First, type 3 in broker-throughput was buggy (see #425). Second, the old code had detected this broken input data and complained, because it generated multiple table keys with the same value. I've added those checks to the new code as well.

Copy link
Member

@ckreibich ckreibich left a comment

Choose a reason for hiding this comment

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

Thanks Dominik — apologies as always for taking a while. Merging now.

@ckreibich
Copy link
Member

Just rebasing here since time has passed. I've tested Zeek's testsuite with this and it's all looking good.

@ckreibich ckreibich merged commit 44cf64b into master Oct 19, 2024
19 checks passed
@ckreibich ckreibich deleted the issue/gh-161 branch October 19, 2024 00:11
ckreibich added a commit to zeek/zeek that referenced this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use safe data format for writing data to backends
2 participants