Redesign upgrade::{write_one, write_with_len_prefix, read_one}

[thomaseizinger]

This PR re-designs the above three functions to:

• allow for better composition
• remove footguns
• make them easier to use

We achieve this by:

1. Deprecating the write_one function

Semantically, this function is a composition of write_with_len_prefix and io.close(). This represents a footgun because the close functionality is not obvious and only mentioned in the docs. Using this function multiple times on a single substream will produces hard to debug behaviour.

2. Deprecating read_one and write_with_len_prefix functions

3. Introducing write_length_prefixed and read_length_prefixed

• These functions are symmetric and do exactly what you would expect, just writing to the socket without closing
• They also have a symmetric interface (no more custom errors, just io::Error)

[mxinden]

Would you mind including an entry in core/CHANGELOG.md as well?

[thomaseizinger]

While we are at it with breaking the interface, I've always found it a bit annoying that read_length_prefixed returns a dedicated error and not just an io::Error. Do you think we could represent the TooLarge variant as one of the representations of io::Error?

[mxinden]

While we are at it with breaking the interface, I've always found it a bit annoying that read_length_prefixed returns a dedicated error and not just an io::Error. Do you think we could represent the TooLarge variant as one of the representations of io::Error?

How about ErrorKind::InvalidData? If you decide to apply this change, can you document the ErrorKind::InvalidData return value?

[thomaseizinger]

@mxinden While writing migration docs, I thought it would actually be much better to just not break the API right away and instead deprecate the old functions. So I did just that! The deprecation note should hopefully guide people towards the new functions.

I removed ReadOneError from the new read_length_prefixed function and returned a custom io::Error with a dedicated error message. Do you still want me to document that as part of the functions rustdoc? I think the error message is good enough that it should lead people towards the problem if they ever hit it.

[mxinden]

Looks good to me. Thanks for thorough patch.

I don't quite understand the benefit of deprecating the 3 methods, given that users will have to update at some point anyways and updating should be very low friction. That said, I am fine either way.

[thomaseizinger]

@mxinden Noticed another thing, please hold on with merging.