Skip to content

Conversation

@0xpr03
Copy link
Contributor

@0xpr03 0xpr03 commented Jun 9, 2021

Simulates the anyhow<thiserror> matryoshka for contexts by
implementing this via kind-parent error types.

Closes #7

@0xpr03
Copy link
Contributor Author

0xpr03 commented Jun 9, 2021

Looking at the details it may be easier to use snafu to get free optional contexts, I'm tried to map your context model onto thiserror, but it's specifically noted that thiserror doesn't come with such a nice .context wrapper out of the box. Snafu does support such context cases, which makes it easier to use but doesn't solve the problem entirely:

The other thing is that you kind of have to use two error types to get a kind and an additional context that can be stacked infinitely from function to function. I kind of simulated this here via FooError and FooErrorKind having a transparent wrapper and a context version.

Simulates the anyhow<thiserror<Error>> matryoshka for contexts by
implementing this via kind-parent error types.

Signed-off-by: Aron Heinecke <aron.heinecke@t-online.de>
Copy link
Owner

@jsvana jsvana left a comment

Choose a reason for hiding this comment

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

I like this overall, but I think it might be cleaner to get rid of Context entirely and instead use more specific thiserror variants. That way we can still get the error message ("failed to read packet ID"), but we get a specific error enum variant (ProtocolError::MalformedPacket(String)). What do you think?

IoError,
#[error(transparent)]
Generic(#[from] ProtocolErrorKind),
#[error("{}: {}",context, source)]
Copy link
Owner

Choose a reason for hiding this comment

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

nit: can you run rustfmt on this?

@0xpr03
Copy link
Contributor Author

0xpr03 commented Jun 10, 2021

but we get a specific error enum variant
ProtocolError::MalformedPacket(String)

What exactly do you want to store in the String ?

@0xpr03
Copy link
Contributor Author

0xpr03 commented Jun 10, 2021

I'd try to preserve the original error. If we're looking at the server module we have

.read_packet()
            .await
            .map_err(|e|ServerError::with_context(e, "failed to read response packet"))?;

Which internally also has 3x a context and InvalidPacketId. Which calls also read_varint that also has a specific ProtocolErrorKind::InvalidVarInt and throws for io::Error.

I think you want to preserve all of them in case of an error (or at least the underlying IO error). So you don't end up with only the top level error.
Thus I'd do something like MalformedPacket(#[from] ProtocolError) which contains all the others (and never loose the original error). For the stacked ProtocolErrors its a little bit harder.

Personally I went with snafu adding some high-level contexts in the top function and adding a Backtrace gated behind a feature, so I can backtrace errors when the blackbox of server doesn't behave as expected.

So I think its up to your liking how much detail you want to expose, how you report errors and where you make a cut and don't report the full error chain (not the crate ;) ).

@jsvana
Copy link
Owner

jsvana commented Jun 10, 2021

That's a fair point. Having that context is great. I think my biggest concern here is that it seems like we're rolling context from scratch. I'm not strictly opposed to the solution you have here, but do you figure we'd have less scaffolding code if we just switched this to snafu?

What exactly do you want to store in the String ?

This was just a weak compromise to have a bit of context without the extra scaffolding :)

@0xpr03
Copy link
Contributor Author

0xpr03 commented Jun 10, 2021

Snafu would allow us to get a context for free (and enforce using a context all the time if we don't add a default conversion). It's a little bit more macro heavy (some people really dislike that, up to you) and we could add an optional backtrace. I'm not sure about a nested context, but it should work.

@0xpr03
Copy link
Contributor Author

0xpr03 commented Jun 10, 2021

I could give a snafu version a run, but that'll probably have to wait until next monday.

@0xpr03 0xpr03 closed this Dec 28, 2021
@0xpr03 0xpr03 deleted the fix_7 branch December 28, 2021 11:43
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.

Use a std::Error based error crate like thiserror

2 participants