Skip to content
This repository has been archived by the owner on Aug 23, 2022. It is now read-only.

Stream::read: partial read OR returning the expected size of the buffer #28

Closed
melekes opened this issue Jul 8, 2022 · 5 comments
Closed
Labels
enhancement New feature or request

Comments

@melekes
Copy link
Contributor

melekes commented Jul 8, 2022

The problem I'm facing right now is where I'm passing a buffer to Stream::read, which is not big enough (Error::ErrShortBuffer). Note there's no indication of the expected size => you're forced to guess the number.

Also note it's different from TcpStream https://doc.rust-lang.org/std/io/trait.Read.html#tymethod.read, which partially reads the data and does not put any restrictions on the size of the buffer (i.e. you, the caller, is in control of how fast you're consuming data).

I can see that the code in Pion is identical to one here, but I nonetheless think we need to change something.

Either

  1. switch to "tcp stream" partial reading model
  2. indicate the expected buffer size by changing ErrShortBuffer to be ErrShortBuffer { min: usize }

Comments and suggestions are welcome.

@melekes melekes added the enhancement New feature or request label Jul 8, 2022
@melekes
Copy link
Contributor Author

melekes commented Jul 8, 2022

There's also talk about auto-tuning buffer size, which is slightly related pion/sctp#218 (comment)

@melekes
Copy link
Contributor Author

melekes commented Jul 8, 2022

self.subtract_num_bytes(to_copy);

also, why are we subtracting n_bytes before we've actually successfully written them to client's buffer?

melekes added a commit to melekes/rust-libp2p that referenced this issue Jul 11, 2022
@algesten
Copy link
Member

I prefer 1. Make it as Rust-y as possible. If we for some reason need to have an in between transition period, we can do 2.

@k0nserv
Copy link
Member

k0nserv commented Jul 11, 2022

I agree with @algesten, matching other Rust APIs, especially from the standard lib is good idea I think

@k0nserv
Copy link
Member

k0nserv commented Aug 23, 2022

We have migrated this crate to the monorepo(webrtc-rs/webrtc) please re-open this issue over there.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants