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

wip: remove send + sync from db tx #4215

Closed
wants to merge 3 commits into from
Closed

Conversation

onbjerg
Copy link
Member

@onbjerg onbjerg commented Aug 15, 2023

An attempt to remove the Send + Sync bounds from database transactions.

Removing these opens up the possibility for us to use e.g. redb, or other databases where write transactions are not necessarily at least Send.

In order to facilitate this, the PR also removes the async portions of the stage trait and the pipeline, instead committing to becoming near-fully synchronous. A few reasons:

  1. Most of the stage code is sync already, and the pipeline is ~sync as well
  2. The only stages that use async in any meaningful capacity are the two downloading stages
  3. Removing async from the traits allows us to avoid pains around await points now that the db tx trait is not Send + Sync

On point 2, @mattsse suggested that we should add a new function to the Stage trait that can execute async code and determine whether the stage is ready for execution, akin to the Service::poll_ready function in tower. For headers and bodies, we would poll the downloaders, put the results into a buffer in the stage itself, and determine whether we are ready to write the buffer to disk (in which case the stage is ready and it is executed sync)

Some pain points:

  1. A lot of our provider stuff use the db tx trait (or the db gat trait)
  2. All of our provider traits (incl factories) have Send + Sync as a bound
  3. Since our db tx's are no longer Send + Sync, things like LatestStateProvider, DatabaseProvider etc. cannot satisfy this bound

Removing the Send + Sync requirement from the provider traits makes everything almost compile, except for RPC.

RPC makes heavy use of the provider traits. Adding Send + Sync where Rustc complains in RPC works, until you encounter code that use the state provider factory.

The state provider factory has functions to get e.g. the latest state, which yields a Box<dyn StateProvider + 'a>. Adding Send to this (as Rustc requires) gives you a new error - the state provider factory cannot fulfull the Send bound when it wants to return a LatestStateProvider (or any other provider that use the db tx trait).

Not entirely sure what direction to go in here yet, and I already nuked all of my Send + Sync bound removal from the providers crate to start fresh

@onbjerg onbjerg added A-staged-sync Related to staged sync (pipelines and stages) C-debt Refactor of code section that is hard to understand or maintain D-complex Quite challenging from either a design or technical perspective Ask for help! A-db Related to the database labels Aug 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-db Related to the database A-staged-sync Related to staged sync (pipelines and stages) C-debt Refactor of code section that is hard to understand or maintain D-complex Quite challenging from either a design or technical perspective Ask for help!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant