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

Use spawn_blocking for dry-run requests #761

Merged
merged 1 commit into from
Nov 8, 2022

Conversation

Voxelot
Copy link
Member

@Voxelot Voxelot commented Nov 5, 2022

fixes: #752

Moves dry_run to a tokio::spawn_blocking call

Since the executor is mostly blocking code, issuing too many dry-run requests could impact more critical functions of the node (such as block production) if executed directly on the main worker pool.

Limits the number of concurrent dry_run requests by the CPU count

This is because in the worst case we will be CPU bound. While some txs may be state-access heavy and I/O bound, other txs could be CPU bound by using lots of crypto ops like hashing and ecrecover. To ensure we don't grind the system to a halt with hundreds of CPU intensive blocking threads, semaphore permits are used to rate limit the number of dry_run requests.

Removes async from the executor

Previously the executor methods were marked as async, even though there were no async calls. This posed a risk when calling the executor within spawn_blocking, as spawn_blocking is a sync context and must use block_on to call any futures. Using block_on is risky because it can easily lead to deadlocks when using async code. While block_on likely would've been safe to use in this one case, it could be the source of difficult-to-debug issues if more asynchronous code is added to the executor in the future.

…e need for block_on, as it doesn't actually need to be async.
@Voxelot Voxelot requested a review from a team November 5, 2022 21:35
Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

The change looks clean. I like the reduction of async in our code=D Now, Executor acts as expected.

We can add a unit test where the semaphore's value is 2. We run 4 dry_run:

  • First dry_run runs forever
  • Second dry_run immediately executed
  • Third dry_run runs forever
  • Fourth dry_run is blocked because the limit is reached(test can wait for timeout).

It is easy to achieve with the mocking of the Executor trait and block or not based on the block_height.

But maybe it is overkill, up to you=)

@Voxelot Voxelot merged commit 1b86ca0 into master Nov 8, 2022
@Voxelot Voxelot deleted the Voxelot/dry-run-blocking-threads branch November 8, 2022 03:38
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.

use semaphore or restricted size threadpool for dry_run block execution
2 participants