Skip to content

Removes async_append_some #283

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 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

mzimbres
Copy link
Collaborator

@anarthal Before I start investigating how to proceed with @D0zee PR I would like to merge this one which simplifies how the read buffer is used.

void commit_append(std::size_t read_size);

[[nodiscard]]
auto get_append_buffer() noexcept -> std::pair<char*, std::size_t>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Consider replacing by boost::span<char>

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 not public, so I avoided pulling in another boost dependency.

*
* Sets a limit on how much data is allowed to be read into the
* read buffer. It can be used to prevent DDOS.
*/
std::size_t max_read_size = (std::numeric_limits<std::size_t>::max)();
std::size_t max_read_buffer_size = (std::numeric_limits<std::size_t>::max)();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a braking change, is there a possibility to maintain the old name?


namespace boost::redis::detail {

class read_buffer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest adding a test_read_buffer unit test that covers these functions, even if they're small


class read_buffer {
public:
void prepare_append(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggest returning the error code with [[nodiscard]]

private:
int resume_point_{0};
read_buffer read_buffer_;
config cfg_;
action action_after_resume_;
action::type next_read_type_ = action::type::append_some;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need a needs_more action and a next_read_type_? It looks like it has the same semantics as append_some.

append_some should likely be renamed to read_some

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

needs_more is useful in some situations

  1. Testing: I know that the fsm is being suspended exactly here.
  2. Logging: Most responses will be contained entirely in the buffer when parsing starts. I would however like to see in the log when parsing has to be suspended because the current message is incomplete and more is needed.

append_some should likely be renamed to read_some

That is reasonable, I can change it.

if (!on_push_) // Prepare for new message.
on_push_ = is_next_push();
on_push_ = is_next_push(data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is likely not something to fix in this PR, but isn't the multiplexer violating point 1. in the comment above? While we're parsing something that's not a push and we run out of data, on_push_ will be false and we will look at the message's first byte even if we shouldn't.

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 have to review this again and test it. I remember it being trick see comments in is_next_push.

{
if (is_push) {
usage_.pushes_received += 1;
usage_.push_bytes_received += parser_.get_consumed();
usage_.push_bytes_received += size;
on_push_ = false;
Copy link
Collaborator

Choose a reason for hiding this comment

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

commit_usage setting on_push looks semantically surprising

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, it is not the correct place but it works well because commit_usage is called only once when finished with an individual response. This has to be moved elsewhere.

void consume(std::size_t size);

private:
std::string buffer_;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be better represented as std::vector<char>, instead. SBO here hurts more than it helps, and the contents are semantically closer to an array of bytes than a string (i.e. traits here is irrelevant).

@@ -34,3 +35,5 @@ void run(
boost::redis::config cfg = make_test_config(),
boost::system::error_code ec = boost::asio::error::operation_aborted,
boost::redis::operation op = boost::redis::operation::receive);

void append_read_data(boost::redis::detail::reader_fsm& fsm, std::string const& data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

string_view?


void append_read_data(boost::redis::detail::reader_fsm& fsm, std::string const& data)
{
auto const buffer = fsm.get_append_buffer();
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe BOOST_ASSERT that we don't exceed the buffer size?

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