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

Add try_fail_point! #68

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add try_fail_point! #68

wants to merge 1 commit into from

Conversation

cgwalters
Copy link

In my project I have a ton of code which uses anyhow::Result. I want a convenient way to force a function to return a stock error from a failpoint. Today this requires something like:

    fail::fail_point!("main", true, |msg| {
        let msg = msg.as_deref().unwrap_or("synthetic error");
        Err(anyhow::anyhow!("{msg}"))
    });

which is cumbersome to copy around.

Now, I conservatively made this a new macro. I am not sure how often the use case of a fail point for an infallible (i.e. non-Result) function occurs. It may make sense to require those to take a distinct inject_point! or something?

@cgwalters
Copy link
Author

So this doesn't actually compile right now, I get:

error[E0277]: the size for values of type `dyn std::error::Error + Send + Sync` cannot be known at compilation time
    --> src/lib.rs:854:19
     |
854  |             Err(e)?;
     |                   ^ doesn't have a size known at compile-time
...
1067 |         try_fail_point!("tryfail");
     |         -------------------------- in this macro invocation

For which I'm a little confused about right now and hoping that this is obvious to you 😄

@BusyJay
Copy link
Member

BusyJay commented Jan 6, 2023

I think that's because compiler can't figure out the correct type for Err(e).

@cgwalters
Copy link
Author

Yeah, it seems like we should be able to structure the code to do this though. Anyways I started to pass 30 minutes of trying to figure it out and decided I should toss up the PR as is to see if the functionality was desired first - does it make sense to you? If so I'll circle back to working on this at some point.

@cgwalters cgwalters force-pushed the try-fail branch 2 times, most recently from 26c0751 to b227f71 Compare January 6, 2023 15:25
@cgwalters
Copy link
Author

OK cool, got this to work! In retrospect it was a bit obvious, I had to make an error type for the crate that can be then converted into other error types.

@cgwalters cgwalters marked this pull request as ready for review January 6, 2023 15:25
In my project I have a ton of code which uses `anyhow::Result`.  I
want a convenient way to force a function to return a stock error
from a failpoint.  Today this requires something like:

```
    fail::fail_point!("main", true, |msg| {
        let msg = msg.as_deref().unwrap_or("synthetic error");
        Err(anyhow::anyhow!("{msg}"))
    });
```
which is cumbersome to copy around.

Now, I conservatively made this a new macro.  I am not sure how
often the use case of a fail point for an infallible (i.e. non-`Result`)
function occurs.  It may make sense to require those to take
a distinct `inject_point!` or something?

Signed-off-by: Colin Walters <[email protected]>
@cgwalters
Copy link
Author

OK, cleaned up even more - we can now (IMO) remove the whole doc section which talks about how to manually deal with Result with fail_point! and just point at try_fail_point!. I also did some other tweaks like supporting std::io::Error directly.

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 12, 2023
When I was debugging https://issues.redhat.com/browse/OCPBUGS-2866
it would have been quite convenient to have a failpoint set up
in the transaction init path, because I could have forcibly injected
a sleep there.

Add one there, and in a few other places I semi-randomly
chose by skimming through the code.  The cost of this is very low,
but if we find other bugs in the future it will likely be useful.

Further, we can and should start using failpoints to forcibly
inject errors in our CI tests.

Since movement on review of tikv/fail-rs#68
has been slow, we just copy the code into our repo for now.
@cgwalters
Copy link
Author

OK for now I copied this macro into our project, see e.g. coreos/rpm-ostree#4259

But since I think the eval bit is an implementation detail, and I'd like to use this in other crates, it'd be nice to have this PR merged when you get a chance; thanks!

cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jan 12, 2023
When I was debugging https://issues.redhat.com/browse/OCPBUGS-2866
it would have been quite convenient to have a failpoint set up
in the transaction init path, because I could have forcibly injected
a sleep there.

Add one there, and in a few other places I semi-randomly
chose by skimming through the code.  The cost of this is very low,
but if we find other bugs in the future it will likely be useful.

Further, we can and should start using failpoints to forcibly
inject errors in our CI tests.

Since movement on review of tikv/fail-rs#68
has been slow, we just copy the code into our repo for now.
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.

2 participants