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

fix(cardano-chain-follower): Refactor cardano-chain-follower to fix client concurrency bug #182

Merged
merged 12 commits into from
Mar 18, 2024

Conversation

FelipeRosa
Copy link

@FelipeRosa FelipeRosa commented Mar 14, 2024

Description

This PR fixes a bug that caused the Follower to get stuck when trying to set its read pointer while waiting for the next chain update.

Related Issue(s)

Closes #180

Description of Changes

Background task

  • Follower background task refactored to allow dropping the client and recreating it when setting the read pointer (which is needed because there seems to be no way of aborting an ongoing request to the node)
  • Removed "read requests" from the follow task so the follow task is now only responsible for fetching chain updates and setting the read pointer when requested

read_block and read_block_range

Follower::read_block and Follower::read_block_range now spawn futures and return join handles to make it possible to make read requests concurrently while not depending on the follower's background task.

New examples

  • Added set_read_pointer example that shows changing the follower's read pointer while following the chain
  • Added concurrent_reads example that shows how to read chain blocks concurrently

Breaking Changes

  • Some variants of the crate's Error type were renamed/removed
  • ReadBlock and ReadBlockRange implement Future and should be awaited directly, i.e.: follower.read_block().await instead of follower.read_block().read().await

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@FelipeRosa FelipeRosa self-assigned this Mar 14, 2024
@FelipeRosa FelipeRosa changed the title fix: Refactor cardano-chain-follower to fix client concurrency bug fix(cardano-chain-follower): Refactor cardano-chain-follower to fix client concurrency bug Mar 14, 2024
@FelipeRosa FelipeRosa added the draft Draft label Mar 14, 2024
@FelipeRosa FelipeRosa force-pushed the fix/cardano-chain-follower-set-read-pointer branch from 51d5f66 to 67bb328 Compare March 15, 2024 13:46
@FelipeRosa FelipeRosa force-pushed the fix/cardano-chain-follower-set-read-pointer branch from 97fe020 to 3357f0a Compare March 15, 2024 18:25
@FelipeRosa FelipeRosa added review me PR is ready for review and removed draft Draft labels Mar 15, 2024
@FelipeRosa FelipeRosa marked this pull request as ready for review March 15, 2024 21:06
Copy link
Contributor

@bkioshn bkioshn left a comment

Choose a reason for hiding this comment

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

Looks good to me. Quite clear after taking a look at the code after the last tech discussion. Just a few grammar error (:

hermes/crates/cardano-chain-follower/src/follow.rs Outdated Show resolved Hide resolved
stevenj
stevenj previously approved these changes Mar 18, 2024
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

LGTM, added suggestions for the grammar issues @bkioshn already mentioned.

hermes/crates/cardano-chain-follower/src/follow.rs Outdated Show resolved Hide resolved
@stevenj stevenj enabled auto-merge (squash) March 18, 2024 06:59
@stevenj stevenj self-requested a review March 18, 2024 06:59
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

LGTM

@stevenj stevenj merged commit a9516fc into main Mar 18, 2024
5 checks passed
@stevenj stevenj deleted the fix/cardano-chain-follower-set-read-pointer branch March 18, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
review me PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cardano chain follower fails to set read pointer
3 participants