Skip to content

Conversation

@0xOmarA
Copy link
Contributor

@0xOmarA 0xOmarA commented Jul 14, 2025

Summary

This PR updates the way in which we handled async tasks and Futures in the codebase exposing the BlockingExecutor which allows for a simple way to execute async tasks in a blocking fashion and from blocking functions.

Description

This PR implements the changes discussed in #40. More specifically, I changed how the codebase handles async tasks and Futures to allow for us to more easily execute async tasks, which we might need soon as seen in #39. We still maintain all of the core characteristics of the previous solution in that the tokio runtime runs in a separate thread and all communication to and from the runtime happens over channels.

This change is heavily inspired by our existing implementation. In fact, one could go as far as saying that it's just syntactic sugar over our previous implementation. It also adds new capabilities to the tokio runtime thread such as allowing it to catch panics gracefully so that a panic in one async task doesn't stop the entire code.

One of the nice things about this change is that we no longer need to define any new structs or trait implementations for tasks that we want to run on this executor, making use of it is as simple as:

fn blocking_function() {
    let result = BlockingExecutor::execute(async move {
        tokio::time::sleep(std::time::Duration::from_secs(1)).await;
        0xFFu8
    })
    .expect("Computation failed");

    assert_eq!(result, 0xFF);
}

@0xOmarA 0xOmarA mentioned this pull request Jul 14, 2025
3 tasks
Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

LGTM! modulo some nits

Cargo.toml Outdated
Comment on lines 81 to 92

# We set the `panic` behavior to `unwind` in both the `release` and `dev` profiles since our async
# runtime attempts to catch panics and it can only catch panics if we compile our code with unwind
# as the panic behavior. For more information, please see the `catch_unwind` documentation where it
# mentions that panics can only be caught if they unwind and not if they abort:
# https://doc.rust-lang.org/std/panic/fn.catch_unwind.html#notes

[profile.release]
panic = "unwind"

[profile.dev]
panic = "unwind"
Copy link
Member

Choose a reason for hiding this comment

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

This should be unnecessary as it's the default anyways?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah good point, I was trying to be more explicit about it but I'm happy to just resort to keeping it implicit.

Comment on lines 94 to 95
// Creating a one-shot channel for this task that will be used to send and receive the
// response of the task.
Copy link
Member

@xermicus xermicus Jul 15, 2025

Choose a reason for hiding this comment

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

Suggested change
// Creating a one-shot channel for this task that will be used to send and receive the
// response of the task.
// Creating a one-shot channel for this task that will be used to send and receive the
// response of the task.

If anything the comment should say why you use a one-shot channel (don't just re-formulate the following code statement in human language).

@0xOmarA
Copy link
Contributor Author

0xOmarA commented Jul 15, 2025

@xermicus Thank you for the review! I realized that I over-commented a lot of parts of the code 😅 I accepted the suggestions you made and went back removing some of the older comments and only keeping comments where they're appropriate and meaningful. Let me know how it looks now

Copy link
Member

@xermicus xermicus left a comment

Choose a reason for hiding this comment

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

Thanks!

@xermicus
Copy link
Member

@xermicus Thank you for the review! I realized that I over-commented a lot of parts of the code 😅 I accepted the suggestions you made and went back removing some of the older comments and only keeping comments where they're appropriate and meaningful. Let me know how it looks now

No worries. Looks much more concise now thank you!

@0xOmarA 0xOmarA added this pull request to the merge queue Jul 15, 2025
Merged via the queue into main with commit 14888f9 Jul 15, 2025
5 checks passed
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