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

Check runtime dynamically #1135

Merged
merged 8 commits into from
Mar 1, 2024
Merged

Check runtime dynamically #1135

merged 8 commits into from
Mar 1, 2024

Conversation

wtdcode
Copy link
Contributor

@wtdcode wtdcode commented Feb 29, 2024

This PR removes the improper initialization of the runtime.

Explanation:

  • Initializing a runtime when creating the struct doesn't make sense.

Users may create an EthersDB in a sync context but later use it in an async environment and vice versa. In this case, two runtimes will conflict. In addition, the current code, by default, creates a multithreaded runtime, which could be unexpected for users. For example, users may want to run many instances in parallel with separate cores pinned but the runtime created could interfere with this design.

Therefore, reasonable behavior is always checking if we are in an async environment by Handle::try_current. If we are in async, we will pick the runtime created by users. Or else, we simply create a current_thread runtime to get things done. Note this is the recommended way by tokio docs.

For current_thread runtime, a new thread is spawned to complete the futures as tokio doesn't allow us to block_in_place, which essentially does spawn_blocking. This could be removed once we have a async equivalent trait.

  • Does it make things slower?

No. Because the current implementation only has concurrency but not parallelism. Specifically, see:

        let f = async {
            let nonce = self.client.get_transaction_count(add, self.block_number);
            let balance = self.client.get_balance(add, self.block_number);
            let code = self.client.get_code(add, self.block_number);
            tokio::join!(nonce, balance, code)
        };
       let (nonce, balance, code) = self.block_on(f);

Either a current_thread or a multi_thread runtime will execute the three rpc calls one by one. Refer to tokio::join! documents for details.

  • Any breaking changes?

No, as explained above.

@wtdcode
Copy link
Contributor Author

wtdcode commented Feb 29, 2024

This also solves #1056 as I tested.

@wtdcode wtdcode marked this pull request as draft February 29, 2024 11:09
Comment on lines 36 to 37
match Handle::try_current() {
Ok(handle) => handle.block_on(f),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will still panic if called from the main Runtime::block_on thread.

what we want here is

tokio::task::block_in_place(move || {
    handle.block_on(f)
});

Copy link
Contributor Author

@wtdcode wtdcode Feb 29, 2024

Choose a reason for hiding this comment

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

Yes, I just noticed the problem. Actually, I think it looks impossible to run a future in a non-async function within a tokio runtime (?.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I tried this works indeed. But block_in_place panic for new_current_thread therefore we have to handle that too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mattsse

I investigated a bit. It looks like we can't support current_thread due to not being able to blocking_in_place without changing the signature of the Database(Ref) traits. I have two options here:

  1. Document clearly that, EthersDB can't be used with current_thread.
  2. Less preferably, we start another thread and create another runtime to do it.

Which one do you prefer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done with 2 so we can support all use cases, could you have a look? @mattsse

Copy link
Member

Choose a reason for hiding this comment

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

There is issue related to this, not sure if you have looked at it: #1056

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I tested this addresses #1056 cleanly.

@wtdcode wtdcode marked this pull request as ready for review February 29, 2024 11:34
@wtdcode wtdcode mentioned this pull request Feb 29, 2024
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.

I think this should work

@rakita
Copy link
Member

rakita commented Mar 1, 2024

Thanks!

@rakita rakita merged commit ef64e4d into bluealloy:main Mar 1, 2024
25 checks passed
@github-actions github-actions bot mentioned this pull request Feb 29, 2024
fubuloubu pushed a commit to ApeWorX/revm that referenced this pull request Apr 11, 2024
* Check runtime dynamically

* Drop futures as clippy suggests

* Fix panic

* Format

* Support current_thread runtime

* Revert unnecessary changes

* Add a few documents

* derive Clone thanks to removing runtime field
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.

None yet

3 participants