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

chore(rpc): trait for blocking task #8538

Merged
merged 25 commits into from
Jun 5, 2024
Merged

Conversation

emhane
Copy link
Member

@emhane emhane commented Jun 1, 2024

Simplifies the interface for executing blocking code in EthApi.

  • Removes semantically counterintuitive function for running a future on a blocking task Moves EthApi::on_blocking_task into trait method SpawnBlocking::spawn_blocking_io, which replaces less general EthTransactions::spawn_blocking. Moves EthApi::spawn_tracing_task_with into SpawnBlocking::spawn_tracing, replacing less general EthTransactions::spawn_blocking.
  • Shrinks scope of code to execute on blocking task, to encompass only sync code, by moving async code into outer scope
  • Moves methods for spawning blocking threads into trait, to access via default trait method impls in EthBlocks and EthTransactions
  • Replaces calls to EthApi::on_blocking_task with calls to eth::api::SpawnBlocking::spawn_blocking_io in impl of EthApiServer. Moves closer to goal of eventually having default trait method impls for EthApiServer.

@emhane emhane added A-rpc Related to the RPC implementation A-op-reth Related to Optimism and op-reth labels Jun 1, 2024
@emhane emhane requested a review from mattsse June 1, 2024 10:27
@emhane emhane changed the base branch from matt/scaffold-ethapi to main June 1, 2024 10:28
@emhane emhane changed the base branch from main to matt/scaffold-ethapi June 1, 2024 10:28
@emhane
Copy link
Member Author

emhane commented Jun 1, 2024

I don't get why this test doesn't work test_ensure_backwards_compatibility. it's unrelated to changes in this pr even, no? @mattsse

besides, it works locally :'(

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this change is problematic because it no longer treats CPU-bound and IO-bound tasks differently and spawns them in the pool of blocking tasks, which is a rayon wrapper intended only for tracing tasks that are predominantly CPU-bound

crates/evm/execution-types/src/bundle.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/traits/blocking_task.rs Outdated Show resolved Hide resolved
crates/rpc/rpc/src/eth/api/mod.rs Show resolved Hide resolved
@emhane emhane requested a review from DaniPopes as a code owner June 5, 2024 00:26
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, trait suggestion

crates/rpc/rpc/src/eth/api/traits/blocking_task.rs Outdated Show resolved Hide resolved
@emhane emhane merged commit 3d42231 into matt/scaffold-ethapi Jun 5, 2024
19 of 25 checks passed
@emhane emhane deleted the emhane/blocking-read branch June 5, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-op-reth Related to Optimism and op-reth A-rpc Related to the RPC implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants