Skip to content

Adds a sans-io fsm to the read operation. #274

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

Merged
merged 1 commit into from
Jun 23, 2025
Merged

Conversation

mzimbres
Copy link
Collaborator

@anarthal: Notes for the code review

  • Adds a fsm for read operation following your work on exec_fsm.
  • Log every event of the reader_fsm after it resumes, which improves log information content.
  • The logger interface has to be extended with types that are actually in the detail namespace, so I make fsm actions a public type, this is needed to interpret log messages. I would like to do this to the exec_fsm too once a log object becomes available for async_exec, so I did not change your code yet.
  • Cancellation hasn't been addresses yet for read operation. This will need more thought and a separate PR.

Example log for the subscriber example. Once all ops are refactored to use an fsm, the log will become very detailed and more helpful

(Boost.Redis) resolve results: 127.0.0.1:6379
(Boost.Redis) connected to 127.0.0.1:6379
(Boost.Redis) fsm action: (reader_action::type::setup_cancellation, 0, Success)
(Boost.Redis) fsm action: (reader_action::type::append_some, 0, Success)
(Boost.Redis) writer_op: 117 bytes written.
(Boost.Redis) fsm action: (reader_action::type::notify_push_receiver, 36, Success)
(Boost.Redis) fsm action: (reader_action::type::append_some, 0, Success)
(Boost.Redis) hello_op: Success
subscribe channel 1
^C
(Boost.Redis) ping_op (4): Operation canceled
(Boost.Redis) check_timeout_op (2): Operation canceled
(Boost.Redis) fsm action: (reader_action::type::cancel_run, 0, Success)
(Boost.Redis) fsm action: (reader_action::type::done, 0, Operation canceled)
(Boost.Redis) writer_op (3): connection is closed.

@mzimbres mzimbres requested a review from anarthal June 16, 2025 21:41
@mzimbres mzimbres changed the title Adds a sans-io fsm for read operation. Adds a sans-io fsm to the read operation. Jun 17, 2025
Copy link
Collaborator

@anarthal anarthal left a comment

Choose a reason for hiding this comment

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

This looks good in general. In the longer run, I'd like to:

  • Avoid the cancel_run action completely in the reader and writer - I believe code would be cleaner if it was the parallel group that handled this.
  • Dismantle async_append_some and merge it into the reader (async_compose has some overhead).

But this is not in scope for this PR.

@mzimbres mzimbres force-pushed the refactoring_clean_code branch from 197bea4 to 7d0871f Compare June 21, 2025 21:30
@mzimbres
Copy link
Collaborator Author

I think I addressed all your points. Please have a look at it again. Are you about to get the log PR merged? I will wait and then rebase on top of your changes so that I can remove the usage of types in detail in the logger interface.

@anarthal
Copy link
Collaborator

Looks good. I've still some work to do, as CIs don't pass yet and there's a ton of tests to write. I think it's better if you merge/squash merge it and I'll move all action types back to detail as part of the logger PR.

@mzimbres mzimbres force-pushed the refactoring_clean_code branch 2 times, most recently from eae25c0 to cfe33f7 Compare June 23, 2025 20:12
@mzimbres mzimbres force-pushed the refactoring_clean_code branch from cfe33f7 to 620b1e9 Compare June 23, 2025 20:24
@mzimbres mzimbres merged commit b822247 into develop Jun 23, 2025
38 checks passed
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