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

[SCTP] Stream::read: partial read OR returning the expected size of the buffer #273

Open
melekes opened this issue Aug 29, 2022 · 6 comments
Labels
help wanted Extra attention is needed subcrate:data For issues specific to the data crate subcrate:sctp For issues specific to the SCTP crate

Comments

@melekes
Copy link
Contributor

melekes commented Aug 29, 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 }

(1) is more "rust" therefore preferred.

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


https://github.com/webrtc-rs/sctp/blob/ed21dae1aa7dc5f37bd40fb344fa19746b932570/src/queue/reassembly_queue.rs#L282

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


Migrated from webrtc-rs/sctp#28

@k0nserv
Copy link
Member

k0nserv commented Aug 29, 2022

From the last issue, the agreement seems to be that we should mimic the TCP implementation. My only concern is whether it would be tricky to use an API like this for the data channels

@k0nserv k0nserv added help wanted Extra attention is needed subcrate:sctp For issues specific to the SCTP crate subcrate:data For issues specific to the data crate labels Sep 1, 2022
@k0nserv
Copy link
Member

k0nserv commented Sep 1, 2022

If anyone is interested in looking at this the first step should be to verify that the change in SCTP would not make its use impossible in the upstream crates. If it turns out to be okay the next step is to implement this

@melekes
Copy link
Contributor Author

melekes commented Sep 30, 2022

I'm looking at this right now.

melekes added a commit that referenced this issue Sep 30, 2022
removes `ErrShortBuffer`

Refs #273
@melekes
Copy link
Contributor Author

melekes commented Sep 30, 2022

If anyone is interested in looking at this the first step should be to verify that the change in SCTP would not make its use impossible in the upstream crates. If it turns out to be okay the next step is to implement this

@k0nserv what is the best way to do it?

opened #304

@k0nserv
Copy link
Member

k0nserv commented Oct 3, 2022

Well, with the merged monorepo I should think that we are okay as long as the code still compiles and the tests all pass 🤔

@melekes
Copy link
Contributor Author

melekes commented Oct 10, 2022

See #304 (comment). Requires more thinking and design.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed subcrate:data For issues specific to the data crate subcrate:sctp For issues specific to the SCTP crate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants