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

support async failpoints #73

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

support async failpoints #73

wants to merge 4 commits into from

Conversation

tabokie
Copy link
Member

@tabokie tabokie commented Oct 25, 2023

  • Added async_fail_point macro, it's usage is exactly the same as fail_point, but must be placed in an asynchronous context.
  • sleep and pause command sent to the async fail point will not block async runtime.
  • In addition, cfg_async_callback is added to inject custom async logic.

Signed-off-by: Xinye [email protected]

Signed-off-by: Xinye <[email protected]>
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated
match task {
Task::Off => {}
Task::Return(s) => return Some(s),
Task::Sleep(_) => panic!(

Choose a reason for hiding this comment

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

I do worry this is not user friendly enough.

Maybe we can spawn a thread and send a signal after sleep some while to implement this without depending on any async runtime.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Signed-off-by: Xinye <[email protected]>
@BusyJay
Copy link
Member

BusyJay commented Oct 25, 2023

Seems duplicated with #58.

@tabokie
Copy link
Member Author

tabokie commented Oct 25, 2023

Seems duplicated with #58.

From the looks of it, #58 doesn't support async sleep and runtime async callback. This PR supports them, but doesn't support specifying a compile-time async callback (the one implemented in-place with fail_point).

I can't figure out an elegant way to unify async and sync version of fail_point!(name, callback). And I'm reluctant to go with #58 and make all fail_point!(name, callback) return async blocks, because most of the time that isn't necessary. If the user want to inject custom async logic, they can always use the more powerful cfg_callback.

Signed-off-by: Xinye <[email protected]>
@waynexia
Copy link

I can't figure out an elegant way to unify async and sync version of fail_point!(name, callback). And I'm reluctant to go with #58 and make all fail_point!(name, callback) return async blocks, because most of the time that isn't necessary. If the user want to inject custom async logic, they can always use the more powerful cfg_callback.

FYI, #58 defines two macros for sync (fail_point!()) and async(async_fail_point!()). It won't affect existing code.

@tabokie
Copy link
Member Author

tabokie commented Oct 25, 2023

I can't figure out an elegant way to unify async and sync version of fail_point!(name, callback). And I'm reluctant to go with #58 and make all fail_point!(name, callback) return async blocks, because most of the time that isn't necessary. If the user want to inject custom async logic, they can always use the more powerful cfg_callback.

FYI, #58 defines two macros for sync (fail_point!()) and async(async_fail_point!()). It won't affect existing code.

I see, it is a slightly different mental model. Your idea is to only use async_fail_point when user want to inject async code. My idea is to always use async_fail_point in async context. In practice the latter is more versatile because we might not know what to inject beforehand.

And continue with my idea, it becomes important to allow user to write async_fail_point(name, || { Ok(()) }) instead of async_fail_point(name, || { async { Ok(()) } }.

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.

4 participants