Getting the `std::io::Error` underlying a `mqtt::Packet::VariablePacketError` is more cumbersome than it should be.

[vincentdephily]

When decoding network bytes into any kind of protocol, even if your error handling strategy is to simply panic, you'll typically want to handle at least the "incomplete packet, need more bytes" error cleanly (by trying again when more data arrived).

I initially handled this by matching the Result of VariablePacket::decode() against VariablePacketError::IoError(ref e) if e.kind() == ErrorKind::UnexpectedEof, but it turns out that the IoError variant can actually end up deeper inside other VariablePacketError variants, depending on how much has been parsed so far.

My current workaround is this (pseudo-code) :

fn to_io_error(e: VariablePacketError) -> std::io::Error {
    match e {
        VariablePacketError::FixedHeaderError(FixedHeaderError::IoError(i))
        | VariablePacketError::IoError(i)
        | VariablePacketError::ConnectPacketError(PacketError::IoError(i))
        | VariablePacketError::ConnackPacketError(PacketError::IoError(i))
        | VariablePacketError::PublishPacketError(PacketError::IoError(i))
        | VariablePacketError::PubackPacketError(PacketError::IoError(i))
        | VariablePacketError::PubrecPacketError(PacketError::IoError(i))
        | VariablePacketError::PubrelPacketError(PacketError::IoError(i))
        | VariablePacketError::PubcompPacketError(PacketError::IoError(i))
        | VariablePacketError::PingreqPacketError(PacketError::IoError(i))
        | VariablePacketError::PingrespPacketError(PacketError::IoError(i))
        | VariablePacketError::SubscribePacketError(PacketError::IoError(i))
        | VariablePacketError::SubackPacketError(PacketError::IoError(i))
        | VariablePacketError::UnsubscribePacketError(PacketError::IoError(i))
        | VariablePacketError::UnsubackPacketError(PacketError::IoError(i))
        | VariablePacketError::DisconnectPacketError(PacketError::IoError(i)) => i,
        _ => std::io::Error::new(ErrorKind::InvalidData, e),
    }
}
pub struct MqttCodec;
impl tokio::codec::Decoder for MqttCodec {
    type Item = VariablePacket;
    type Error = std::io::Error;
    fn decode(&mut self, src: &mut BytesMut) -> Result<Option<Self::Item>, Self::Error> {
        match VariablePacket::decode(src) {
            Ok(vp) => Ok(Some(vp)),
            Err(vperr) => {
                let ioerr = to_io_error(vperr);
                match ioerr.kind() {
                    ErrorKind::UnexpectedEof => Ok(None),
                    _ => Err(ioerr),
                }
            },
        }
    }
}

It works for my current unit tests, but:

• It's surprising and ugly
• It doesn't handle any deeper cases, like VariablePaketError -> PacketError -> VariableHeaderError -> TopicNameError -> StringEncodeError- > IoError (!)
• It could easily bitrot if new enum variants are added.

Maybe there's a better technique ?

I see a few different ways to fix this (and can devote some time towards a PR):

• Make sure that any io::Error bubbles up to the topmost mqtt error type, so the user never has to dig. That's the least-surprising option, I guess it could be implemented by tweaking all the From<SubError> for TopError impls, and we could still retain the deepest error inside .source().
• Implement From<AnyMqttErrorType> for std::io::Error. This is basically a clean version of my workaround, might be useful in other cases, but users might still be surprised that matching on TopMostMqttError::IoError doesn't catch all cases.
• Implement AnyMqttError::io_error() -> Option<Error>. Meh.
• Combine these options and change the error types, for example by always returning an std::io::Error with the mqtt-specific issues in source(). Tempting, but a breaking change and it'll take a while to figure out.

Thanks in advance.

[zonyitoo]

I personally prefer your first proposal:

Make sure that any io::Error bubbles up to the topmost mqtt error type, so the user never has to dig.

Which one do you prefer?

[vincentdephily]

Yes, 1st proposal is probably the best minimal thing we can do, it fixes the issue without touching the API. Proposal 2 might be a good addendum.

I'll have a look at it this week. I haven't looked at the code yet. Any pointers ?

[zonyitoo]

This is the VariablePacketError https://github.com/zonyitoo/mqtt-rs/blob/master/src/packet/mod.rs#L329

For 1st proposal, just modify those From<...Error> for VariablePacketError, match them and extract the io::Error out and put into VariablePacketError::IoError. For 2nd proposal, just add one more From impl for VariablePacketError.

[vincentdephily]

I've spent a good chunk of this week getting acquainted with the code and trying to implement the first proposal. I've only had partial success, as some of the cases are pretty hard to get at, especially the payload errors.

At this stage I think it'd be saner to go with proposal 4 and accept the API break. I'm thinking of reducing the graph down to two levels so you'd only have this PacketError without type parameter:

pub enum PacketError {
    IoError(io::Error),
    FixedHeaderError(FixedHeaderError),
    VariableHeaderError(ControlType, VariableHeaderError),
    StringEncodeError(ControlType, StringEncodeError),
    TopicNameError(ControlType, TopicNameError),
    InvalidSubscribeReturnCode(ControlType, u8),
    InvalidQualityOfService(ControlType, u8),
    TopicFilterError(ControlType, TopicFilterError),
}

That plan isn't fully-formed yet, but I believe it would simplify both the mqtt-rs implementation and usage without losing information compared to current error types.

What do you think ?

Not sure what the current stability promise is, do we want to do an intermediate release with deprecations and/or use the occasion for other breaking changes (dropping ConnectPacket::new()'s first argument and switching to rust2018 come to mind) ?

[zonyitoo]

Not sure what the current stability promise is

I don't know who is using this library. But I think it is ok to release a breaking change by increasing a primary version number.

And also, yes, migrating to rust2018 is Ok.

[vincentdephily]

Have a look at the PR and tell me what you think.

These are semver-breaking changes, so we probably want to wait a bit to see if we can fit a few more in before making a release.

Thanks.