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

perf(katana-rpc): spawn blocking tasks #1456

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

kariy
Copy link
Member

@kariy kariy commented Jan 18, 2024

Resolves #1448

Creates new crate under katana, katana-tasks, for managing spawning blocking tasks. RPC calls that mostly perform blocking tasks are now sent to their designated threadpools and won't block the async threads.

  • TokioTaskSpawner: mainly for spawning blocking IO-bound tasks (ie reading from storage)
  • BlockingThreadPool: mainly for spawning expensive CPU-bound tasks

Depends on #1455 because now the RPC requests (that used to block the thread before) have to wait for the blocking tasks to finish and thus may be idling for more than 2 seconds which will result in a connection timeout.

Doing sozo migrate on 2s timeout will failed when calling /esimateFee for estimating the World contract declare tx with error connection closed before message completed.

Raw error message from sozo against the new changes:

Caused by:
    Failed to deploy world: Failed to migrate world: Migrator(Provider(Other(TransportError(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(5050), path: "/", query: None, fragment: None }, source: hyper::Error(IncompleteMessage) })))))

@kariy
Copy link
Member Author

kariy commented Jan 18, 2024

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2024

Codecov Report

Attention: 225 lines in your changes are missing coverage. Please review.

Comparison is base (c1de18e) 68.51% compared to head (9abf594) 68.58%.

Files Patch % Lines
crates/katana/rpc/src/starknet.rs 45.64% 212 Missing ⚠️
crates/katana/tasks/src/lib.rs 89.13% 10 Missing ⚠️
crates/katana/core/src/sequencer.rs 40.00% 3 Missing ⚠️
Additional details and impacted files
@@                                      Coverage Diff                                       @@
##           01-18-Increase_katana_RPC_request_timeout_from_2_-_20_secs    #1456      +/-   ##
==============================================================================================
+ Coverage                                                       68.51%   68.58%   +0.06%     
==============================================================================================
  Files                                                             220      221       +1     
  Lines                                                           21690    21881     +191     
==============================================================================================
+ Hits                                                            14861    15007     +146     
- Misses                                                           6829     6874      +45     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kariy kariy requested review from tarrencev and glihm January 18, 2024 12:17
@kariy
Copy link
Member Author

kariy commented Jan 18, 2024

Merge activity

  • Jan 18, 10:24 AM: @kariy started a stack merge that includes this pull request via Graphite.
  • Jan 18, 10:25 AM: Graphite rebased this pull request as part of a merge.
  • Jan 18, 10:27 AM: @kariy merged this pull request with Graphite.

Base automatically changed from 01-18-Increase_katana_RPC_request_timeout_from_2_-_20_secs to dev/katana January 18, 2024 15:25
kariy added a commit that referenced this pull request Jan 18, 2024
Refer to #1456 for full context.

The new timeout value has been chosen arbitrarily, I just thought it seems reasonable enough.
@kariy kariy force-pushed the 01-18-perf_katana-rpc_spawn_blocking_tasks branch from 9abf594 to 64942e1 Compare January 18, 2024 15:25
@kariy kariy merged commit 461040e into dev/katana Jan 18, 2024
10 checks passed
@kariy kariy deleted the 01-18-perf_katana-rpc_spawn_blocking_tasks branch January 18, 2024 15:27
kariy added a commit that referenced this pull request Jan 18, 2024
Refer to #1456 for full context.

The new timeout value has been chosen arbitrarily, I just thought it seems reasonable enough.
kariy added a commit that referenced this pull request Jan 18, 2024
Resolves #1448

Creates new crate under `katana`, `katana-tasks`, for managing spawning blocking tasks. RPC calls that mostly perform blocking tasks are now sent to their designated threadpools and won't block the async threads.

- `TokioTaskSpawner`: mainly for spawning blocking IO-bound tasks (ie reading from storage)
- `BlockingThreadPool`: mainly for spawning expensive CPU-bound tasks

Depends on #1455 because now the RPC requests (that used to block the thread before) have to wait for the blocking tasks to finish and thus may be idling for more than 2 seconds which will result in a connection timeout.

Doing `sozo migrate` on 2s timeout will failed when calling `/esimateFee` for estimating the World contract declare tx with error `connection closed before message completed`.

Raw error message from `sozo` against the new changes:
```console
Caused by:
    Failed to deploy world: Failed to migrate world: Migrator(Provider(Other(TransportError(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(5050), path: "/", query: None, fragment: None }, source: hyper::Error(IncompleteMessage) })))))
```
kariy added a commit that referenced this pull request Jan 19, 2024
Refer to #1456 for full context.

The new timeout value has been chosen arbitrarily, I just thought it seems reasonable enough.
kariy added a commit that referenced this pull request Jan 19, 2024
Resolves #1448

Creates new crate under `katana`, `katana-tasks`, for managing spawning blocking tasks. RPC calls that mostly perform blocking tasks are now sent to their designated threadpools and won't block the async threads.

- `TokioTaskSpawner`: mainly for spawning blocking IO-bound tasks (ie reading from storage)
- `BlockingThreadPool`: mainly for spawning expensive CPU-bound tasks

Depends on #1455 because now the RPC requests (that used to block the thread before) have to wait for the blocking tasks to finish and thus may be idling for more than 2 seconds which will result in a connection timeout.

Doing `sozo migrate` on 2s timeout will failed when calling `/esimateFee` for estimating the World contract declare tx with error `connection closed before message completed`.

Raw error message from `sozo` against the new changes:
```console
Caused by:
    Failed to deploy world: Failed to migrate world: Migrator(Provider(Other(TransportError(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(5050), path: "/", query: None, fragment: None }, source: hyper::Error(IncompleteMessage) })))))
```
@kariy kariy mentioned this pull request Jan 22, 2024
kariy added a commit that referenced this pull request Jan 22, 2024
Refer to #1456 for full context.

The new timeout value has been chosen arbitrarily, I just thought it seems reasonable enough.
kariy added a commit that referenced this pull request Jan 22, 2024
Resolves #1448

Creates new crate under `katana`, `katana-tasks`, for managing spawning blocking tasks. RPC calls that mostly perform blocking tasks are now sent to their designated threadpools and won't block the async threads.

- `TokioTaskSpawner`: mainly for spawning blocking IO-bound tasks (ie reading from storage)
- `BlockingThreadPool`: mainly for spawning expensive CPU-bound tasks

Depends on #1455 because now the RPC requests (that used to block the thread before) have to wait for the blocking tasks to finish and thus may be idling for more than 2 seconds which will result in a connection timeout.

Doing `sozo migrate` on 2s timeout will failed when calling `/esimateFee` for estimating the World contract declare tx with error `connection closed before message completed`.

Raw error message from `sozo` against the new changes:
```console
Caused by:
    Failed to deploy world: Failed to migrate world: Migrator(Provider(Other(TransportError(Reqwest(reqwest::Error { kind: Request, url: Url { scheme: "http", cannot_be_a_base: false, username: "", password: None, host: Some(Domain("localhost")), port: Some(5050), path: "/", query: None, fragment: None }, source: hyper::Error(IncompleteMessage) })))))
```
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.

3 participants