Replace anyhow with thiserror #115

matthiasbeyer · 2022-11-11T15:09:26Z

As this is a library crate, it should not use anyhow.
It should rather use thiserror and define actual error types, that a user can then handle properly and as they think it is best for them.

This PR refactors the codebase to use thiserror rather than anyhow.

Some of these error kinds are still not nice, but I guess it is a starting point. Please tell me what you think!

rkuhn · 2022-11-15T12:31:59Z

After fixing the divergence between master and the published crates (by merging #113), there are now some conflicts with this rather widespread change. I agree that it is a good thing to do, thanks for pushing for it! Will do a full PR review now.

rkuhn

Great work! I added some messaging clarifications. Would you be up for rebasing onto current master?

banyan/src/error.rs

banyan/src/forest/mod.rs

rkuhn · 2022-11-15T12:46:21Z

banyan/src/store/mem_store.rs

-    pub fn into_inner(self) -> anyhow::Result<FnvHashMap<L, Box<[u8]>>> {
-        let inner = Arc::try_unwrap(self.0).map_err(|_| anyhow!("busy"))?;
+    pub fn into_inner(self) -> Result<FnvHashMap<L, Box<[u8]>>, Error> {
+        let inner = Arc::try_unwrap(self.0).map_err(|_| Error::Busy)?;


This could be made clearer, for example Error::MemStore("multiple strong references")

rkuhn · 2022-11-15T12:47:08Z

banyan/src/store/mem_store.rs

        let digest = (self.0.digest)(&data);
        let len = data.len();
        let mut blocks = self.0.blocks.lock();
        if blocks.current_size + data.len() > self.0.max_size {
-            anyhow::bail!("full");
+            return Err(Error::Full)


… then this one could be Error::MemStore("max_size would be exceeded")

You would consolidate the "multiple strong references" and the "max_size would be exceeded" error variants, as it seems. I'd rather keep them seperate, but make the error variants more expressive, to explain what happened...

See my patches 😉

rkuhn · 2022-11-15T12:47:50Z

banyan/src/store/mem_store.rs

        if let Some(value) = self.get0(link) {
            Ok(value)
        } else {
-            Err(anyhow!("not there"))
+            Err(Error::NotThere)


This will be relevant also for external implementations, right? So perhaps Error::BlockNotFound.

So why return an error instead of an Option here? A Option expresses clearly what happened...

This comes from the ReadOnlyStore signature, which should still allow for real errors, I think.

banyan/src/tree.rs

banyan/src/error.rs

matthiasbeyer · 2022-11-15T14:01:41Z

Thanks for the in-depth review! I cannot do the fixes right now. Maybe later today, if not then tomorrow! I am eager to get this in! 👍

matthiasbeyer · 2022-11-16T07:33:29Z

I am about to rebase this btw

matthiasbeyer · 2022-11-16T07:58:49Z

Okay.

So the banyan crate builds for me. The problem is that the BlockWriter and ReadOnlyStore traits (and probably others as well) are not yet usable outside of the banyan crate, as they return banyan::error::Error.

Normally I would add associated types for the error type here, but that makes the whole codebase even more generic, as I would need to add them everywhere.
This would obviously be the right solution, although it is not the easy one.

Another way would be to add a "catchall" error variant in the banyan::error::Error type that contains a Box<dyn std::error::Error>. That would be ugly, but easy.

What do you think?

rkuhn

Regarding the put/get errors: that code should be free to return any type of error it finds useful. I don’t like dyn Error very much, but the alternative is to add two enum variants and make it Error<PutError, GetError>, possibly held in an associated type (e.g. for Transaction — which could with Rust 1.65 be lifted to the Result level, like fn(...) -> Self::Result<()>).

impl<...> Transaction<...> {
    type Result<T> = Result<T, Error<W::Error, R::Error>>;

   ...
}

I’m on the fence. The second approach could also benefit from adding

impl<T, U> From<Error<Void, Void>> for Error<T, U> {
    fn from(...) -> Self {
        // map genuine Banyan errors, as foreign errors are guaranteed absent
    }
}

(to be used in code that calls neither put nor get)

If you want to try out the shiny new GAT feature of Rust 1.65 then feel free! Otherwise, I’d also agree if you said that it is too much effort — the difference lies in how (reliably) users of Banyan can extract underlying store errors to react programmatically.

banyan/src/forest/read.rs

rkuhn · 2022-11-16T15:35:15Z

banyan/src/forest/read.rs

@@ -502,7 +508,10 @@ where
                        offset -= child.count();
                    }
                }
-                Err(anyhow!("index out of bounds: {}", offset))
+                Err(Error::IndexOutOfBounds {
+                    length: node.children.len(),


since we’re iterating over node.children already above, how about summing up length at the same time?

There's no need, self.children.len() is slice::len() which is const and O(1) (if I read this correctly), so this won't cost anything AFAICS! 🤔

ah, but then it’s not the right .len() to compare to!

You mean I need to sum up child.count() and use that as length?

yes, because that is what failed

rkuhn · 2022-11-16T15:36:50Z

banyan/src/store/mem_store.rs

        if let Some(value) = self.get0(link) {
            Ok(value)
        } else {
-            Err(anyhow!("not there"))
+            Err(Error::NotThere)


This comes from the ReadOnlyStore signature, which should still allow for real errors, I think.

banyan/src/tree.rs

matthiasbeyer · 2022-11-17T07:36:40Z

I don’t like dyn Error very much, but the alternative is to add two enum variants and make it Error<PutError, GetError>, possibly held in an associated type

There's the third alternative:

Make the BlockWriter and ReadOnlyStore traits have an associated type:

trait BlockWriter {
    type Error;
    // ...
}

And use our own error type only for places where we implement these traits within the codebase. The associated type might need a : From<banyan::error::Error> bound, but that's not too much of an issue, IMO.

I don't think that's too much effort. Quite contrary, I think this is actually the way to go! Just wanted to make sure whether you're okay with this! 😄

wrt your annotations: Will push fixes in a few minutes.

rkuhn · 2022-11-22T14:30:18Z

And use our own error type only for places where we implement these traits within the codebase. The associated type might need a : Frombanyan::error::Error bound, but that's not too much of an issue, IMO.

We were talking about almost the same thing :-) My proposal only has the wrapping done the other way around: the Banyan library functions (like Transaction::put_block) return Banyan errors, which may contain errors from the underlying BlockWriter implementation. So instead of BlockWriter::Error: From<banyan::error::Error> I’d add Error::BlockWriter(W::Error) to the banyan Error enum. This avoid potential issues where a function needs to return either a writer or a reader error — simply wrap both of them in banyan’s Error and be done with it.

rkuhn · 2022-12-02T08:36:47Z

Are you still working on this? I’d like to publish 0.18 next week, and I could also help push this over the finish line if you want.

matthiasbeyer · 2022-12-02T08:44:30Z

Rebased, squashed away my fixups and formatted the codebase.
Lets see what CI tells us.

Thanks for pinging me on this!

matthiasbeyer · 2022-12-02T09:03:14Z

And some more things to make clippy happy. Please have a look at 6e04476 because I think there might be a bug there...

matthiasbeyer · 2022-12-02T09:21:48Z

🤔 why does clippy succeed locally but fail on actions? What am I missing here?

rkuhn · 2022-12-02T09:25:13Z

Probably a different version: clippy gets new warnings in every Rust version … The CI setup doesn’t specify a version, so I guess stable is used (which should be 1.65 today).

matthiasbeyer · 2022-12-02T09:31:36Z

Weird. I am using nightly here, which is IIRC the one specified for the clippy step. I will double check tomorrow, am on the road today.

rkuhn · 2022-12-03T16:00:54Z

And some more things to make clippy happy. Please have a look at 6e04476 because I think there might be a bug there...

Yes, the new code looks more reasonable than the original, good catch!

Signed-off-by: Matthias Beyer <[email protected]>

By adding a "available" length field that can be shown in the error display. For this, the ZstdDagCborSeq::len() function was added. Signed-off-by: Matthias Beyer <[email protected]>

The error variant is not nice yet, but better than before. Signed-off-by: Matthias Beyer <[email protected]>

This patch simplifies the boolean expressions. Before this patch, the expressions were ported from the `anyhow::ensure!()` macro by replacing it with `if !(...)`, which was simple and a no-brainer to replace. With this patch, we simplify the expression to not contain unnecessary `!` in there. Signed-off-by: Matthias Beyer <[email protected]>

Signed-off-by: Matthias Beyer <[email protected]>

rkuhn · 2022-12-22T11:35:34Z

Hi @matthiasbeyer, do you plan on adding the PutError and GetError variants to the new Error type? As far as I can see this PR cannot yet be merged as is, right?

matthiasbeyer · 2022-12-22T15:42:30Z

Hi.

Yeah... so what I've done (not published yet) is trying to get the banyan-utils crate to build as well... but because of the change of interfaces in the banyan crate, this essentially means to rewrite the whole banyan-utils codebase, which is one big mess (the rewrite, not the code).

I'm kinda stuck in the middle of it, actually... help would be much appreciated, TBH! 😆

rkuhn · 2022-12-22T15:44:04Z

I’m currently incapacitated, but if you push your code somewhere I could take a look after Christmas.

This patch rewrites banyan-utils to use thiserror and the new (thiserror-based) banyan API instead of anyhow.

matthiasbeyer · 2022-12-22T15:46:27Z

Sure, here it is. This commit does not build and is not complete and needs some work and cleanup (and possible needs to be split up into more detailed, individual commits), of course!

rkuhn · 2023-01-06T10:54:12Z

Turns out I wasn’t able to work on this over the Christmas break, and it is unlikely that I’ll be able to before Jan 19, sorry about that.

matthiasbeyer · 2023-01-06T16:17:35Z

No problem, take your time!

rkuhn reviewed Nov 15, 2022

View reviewed changes

matthiasbeyer force-pushed the error-handling branch 2 times, most recently from ac9b424 to a74c92d Compare November 16, 2022 07:56

rkuhn reviewed Nov 16, 2022

View reviewed changes

matthiasbeyer force-pushed the error-handling branch from 417b306 to 5a158a5 Compare December 2, 2022 08:44

matthiasbeyer mentioned this pull request Dec 3, 2022

Install rust 1.65.0 #117

Merged

matthiasbeyer added 11 commits December 4, 2022 18:31

Replace anyhow with thiserror

54b59c2

Signed-off-by: Matthias Beyer <[email protected]>

Make IndexOutOfBounds error kind more expressive

dedc980

By adding a "available" length field that can be shown in the error display. For this, the ZstdDagCborSeq::len() function was added. Signed-off-by: Matthias Beyer <[email protected]>

Make Error::Invalid more expressive

1b4e521

The error variant is not nice yet, but better than before. Signed-off-by: Matthias Beyer <[email protected]>

Replace Busy variant with MultipleStrongRef

54f589c

Signed-off-by: Matthias Beyer <[email protected]>

Replace Full with MaxSizeExceeded error variant

b1f92e0

Signed-off-by: Matthias Beyer <[email protected]>

Rename: FoundPurgedData -> PurgedDataFound

adf47d6

Signed-off-by: Matthias Beyer <[email protected]>

Make error text more expressive

d2145e6

Signed-off-by: Matthias Beyer <[email protected]>

Fix clippy: Do not pass ref here

30ed94f

Signed-off-by: Matthias Beyer <[email protected]>

Simplify comparison

b48c6f5

Signed-off-by: Matthias Beyer <[email protected]>

Fix: Make comparison fit error message

293e515

Signed-off-by: Matthias Beyer <[email protected]>

Remove unnecessary cast

de35a90

Signed-off-by: Matthias Beyer <[email protected]>

matthiasbeyer force-pushed the error-handling branch from d9054de to de35a90 Compare December 4, 2022 17:31

WIP: Rewrite banyan-utils to replace anyhow with thiserror

dde0b3a

This patch rewrites banyan-utils to use thiserror and the new (thiserror-based) banyan API instead of anyhow.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace anyhow with thiserror #115

Replace anyhow with thiserror #115

matthiasbeyer commented Nov 11, 2022

rkuhn commented Nov 15, 2022

rkuhn left a comment

rkuhn Nov 15, 2022

rkuhn Nov 15, 2022

matthiasbeyer Nov 16, 2022

rkuhn Nov 15, 2022

matthiasbeyer Nov 16, 2022

rkuhn Nov 16, 2022

matthiasbeyer commented Nov 15, 2022 •

edited

Loading

matthiasbeyer commented Nov 16, 2022

matthiasbeyer commented Nov 16, 2022

rkuhn left a comment

rkuhn Nov 16, 2022

matthiasbeyer Nov 17, 2022 •

edited

Loading

rkuhn Nov 17, 2022

matthiasbeyer Nov 17, 2022

rkuhn Nov 17, 2022

rkuhn Nov 16, 2022

matthiasbeyer commented Nov 17, 2022

rkuhn commented Nov 22, 2022

rkuhn commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022

rkuhn commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022 via email

rkuhn commented Dec 3, 2022

rkuhn commented Dec 22, 2022

matthiasbeyer commented Dec 22, 2022

rkuhn commented Dec 22, 2022

matthiasbeyer commented Dec 22, 2022

rkuhn commented Jan 6, 2023

matthiasbeyer commented Jan 6, 2023

Replace anyhow with thiserror #115

Are you sure you want to change the base?

Replace anyhow with thiserror #115

Conversation

matthiasbeyer commented Nov 11, 2022

rkuhn commented Nov 15, 2022

rkuhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbeyer commented Nov 15, 2022 • edited Loading

matthiasbeyer commented Nov 16, 2022

matthiasbeyer commented Nov 16, 2022

rkuhn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbeyer Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthiasbeyer commented Nov 17, 2022

rkuhn commented Nov 22, 2022

rkuhn commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022

rkuhn commented Dec 2, 2022

matthiasbeyer commented Dec 2, 2022 via email

rkuhn commented Dec 3, 2022

rkuhn commented Dec 22, 2022

matthiasbeyer commented Dec 22, 2022

rkuhn commented Dec 22, 2022

matthiasbeyer commented Dec 22, 2022

rkuhn commented Jan 6, 2023

matthiasbeyer commented Jan 6, 2023

matthiasbeyer commented Nov 15, 2022 •

edited

Loading

matthiasbeyer Nov 17, 2022 •

edited

Loading