read_varint should not return 0 upon EOF

[ShahakShama]

Description

from read_varint's documentation:

As a special exception, if the socket is empty and EOFs right at the beginning, then we return Ok(0).

I suggest that we change read_varint's behaviour so that EOF will not return 0.

Motivation

There should be a way to distinguish between EOF at the beginning of the socket and the case where there's an actual 0 on the socket.

Current Implementation

The cleanest way to implement this is to change read_varint's return type from impl Future<Output = Result<usize, Error>> to impl Future<Output = Result<Option<usize>, Error>> and an EOF will translate into Ok(None). That's because EOF is a positive flow, not an error, so it should be in the Ok variant.

We'll also need to change the function read_length_prefixed to return an Option as well. It should return Ok(None) if the call to read_varint returned None

If we don't want to break backward compatibility, we can create new functions that will return Option and let the user decide whether to distinguish between 0 and EOF or not to handle with Options. A few suggestions for the new function's name:

• read_varint_if_exists
• read_varint_if_no_eof
• try_read_varint

Are you planning to do it yourself in a pull request?

Yes

[mxinden]

I appreciate the detailed report.

I agree that your proposal would be a cleaner API.

Within rust-libp2p read_varint is only used in read_length_prefixed and as far as I can tell the above special case does not lead to any misbehavior within read_length_prefixed.

Overall, especially since we have unsigned_varint::UviBytes, I no longer exposing read_varint instead of fixing the strange behavior.

[thomaseizinger]

I want to second Max here. These functions are currently exposed in the public API so changing their signature is breaking. But, the good news is, this entire module is scheduled to be deprecated. The relevant issue is here: #4011

There is only one remaining item left after which we can deprecate these functions and remove them in the next breaking release! :)

Does that sound like a meaningful path forward to you? Contributions would be very welcome!

[ShahakShama]

Is the plan to move this module to a different crate or to erase it completely/make it private?
If it's the latter, then I'm currently using it. Should I use a different function instead?

[ShahakShama]

@mxinden the read_varint usage in read_length_prefixed does lead to unwanted behaviour. That's the original issue I had.
My original issue was that when I used read_length_prefixed when the socket reached EOF I got an empty vector (which can be a valid result that the other peer could've sent in my case)

[thomaseizinger]

Is the plan to move this module to a different crate or to erase it completely/make it private?
If it's the latter, then I'm currently using it. Should I use a different function instead?

Yeah, it is recommended that you use https://github.com/mxinden/asynchronous-codec instead. In particular https://docs.rs/unsigned-varint/latest/unsigned_varint/codec/struct.UviBytes.html.

[thomaseizinger]

Is the plan to move this module to a different crate or to erase it completely/make it private?

You are obviously also welcome to fork them. The main reason we are removing them is because they unnecessarily add to our API surface.

[ShahakShama]

Thanks!
I see that UviBytes only covers read_varint. What about read_length_prefixed? where will I be able to find an implementation for that once it's deprecated?

[thomaseizinger]

where will I be able to find an implementation for that once it's deprecated?

See https://github.com/mxinden/asynchronous-codec/blob/master/src/codec/length.rs :)

[ShahakShama]

Thanks :)
I'm closing this issue