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

Simpler errors #35

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

Conversation

vincentdephily
Copy link

Fixes #34

This is still a work in progress (I'd like to simplify a couple more types, investigate some potential bugs I noticed, and maybe add a test or two), but I'd like a review at this stage, particularly of the last commit. You might find the commit message, PacketError doc, and errortypes.svg useful.

Thanks.

Proptest generates random legal packets of all kinds, and reduces discovered failures to the
simplest case. It's particularly useful to explore all error-handling corners.

This commit uses generated packets for an encode/decode rountrip test (which all work) and an
UnexpectedEof test (which currently all fail).

Adding the proptest dev dependency raises the rust version requirement from 1.30 to 1.32.
This is a partial fix for zonyitoo#34 but I didn't manage to fix
all spots. At this stage, I think that flattening the graph of error types would be the saner thing
to do, although it involves an API break.
Using rustfmt 1.2.2-nightly. Better do it all in one go. Better still would be to do it for each
commit from now on ;)
It wasn't used anywhere anymore.
This is the first step in simplifying the error types. The payload encode methods that returned a
generic `PayloadError` now return a concrete type, that can easily be converted into a
`PacketError`. The `PacketError` enum gained variants and `impl From<>` for all those.

I couldn't get the generic `impl<T: Packet + fmt::Debug + 'static> Encodable for T` to compile
(something about Struct->Trait->Trait->Method being harder to resolve when said Struct is in an enum
variant), but using the macro to generate the equivalent works nicely.

This commit breaks the API for users that looked into the error type instead of simply bailing
out. Due to the difficulty of looking into this type, I doubt that anybody was doing this correctly
anyway, but this is nevertheless an API change to ducument.
Use `PacketError` instead. Yay to code deduplication :)

To keep the functionality as-is, I added a `Vec<u8>` to `FixedHeaderError::Unrecognized/Reserved`,
that holds the remaining bytes. Consuming those bytes doesn't seem to be done consistently (for
example it's only done if you decode as a `VariablePacket`), and no unittest take notice of the
issue. Will need to take a second look at those later.
This method has been soft-deprecated in favor of Display trait (which we also implement) since rust
1.27. Remove it to to reduce maintenance cost.
The general philosophy was to consistently bubble common errors like `io::Error` and
`StringEncodeError` up to the parent error, eventually reaching `PacketError`. In doing so, various
error types and variants naturally disappeared, the code got simpler, and some unused enum variants
were deleted.

A few error variants/enum were documented, and a `errotypes.dot` file was added to document the
current error graph.

This scratches my original itch of easily recognizing "more bytes needed to finish parsing" errors,
but it does lose some information like "what packet type was being treated when we reached an
utf8-encoding error ?". I've tried various approaches, but it's tricky to get working without
relying on some error-chaining crate like `snafu` or `failure`, which might not be acceptable for
this lib (and I don't think we want to go back to a `PacketError<Packet>` type). Losing that context
is IMHO ok (there's not a lot that can be done with the context anyway), but a survey of usecases
would be nice to confirm that opinion.
…ype variants.

There's 4 bits, 16 values possible, all acounted for. Don't try to handle impossible values.
* Brought `PacketTypeError::InvalidFlag` directly into `FixedHeaderError`.
* No longer attempting to read the rest of the packet when a `FixedHeaderError` happens.
* Expanded `FixedHeaderError` documentation.

The second bullet point might be controversial as it removes a feature that might be in use. But
reading the rest of the packet is a spec violation (user should close the connection instead), it
degrades performance, complicates code, and seems like a risky bet whichever way you look at it. So
I left the parsed type/flag/lenght in place in case some adventurous user wants to read/parse the
rest of the stream, but made it clear that the user is on his own if he does that.
Since it happens during both encoding and decoding. Minor nitpick, but while we're changing all the
error types...
@vincentdephily vincentdephily changed the title WIP Simpler errors Simpler errors Jun 28, 2019
@vincentdephily
Copy link
Author

I'll stop there, unless you have some comments or suggestions. There's a good bit more that could be done, but I've already spent more time than I expected on this and need to move on.

One alternative we could investigate is using a crate like snafu to chain errors and get much more details, but pragmatically I think the current info is all you need. I'll be dogfooding this branch in my own app next, we'll see.

@zonyitoo
Copy link
Owner

Great job! LGTM.

@zonyitoo
Copy link
Owner

Looking forward to see your further improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants