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

Handle io.ErrShortBuffer error #719

Merged
merged 2 commits into from
Jul 12, 2019
Merged

Handle io.ErrShortBuffer error #719

merged 2 commits into from
Jul 12, 2019

Conversation

cohosh
Copy link
Contributor

@cohosh cohosh commented Jun 19, 2019

Handle an io.ErrorShortBuffer return from ReadDataChannel by doubling
the size of the buffer and making another read.

Description

When ReadDataChannel returns an io,ErrShortBuffer error, a subsequent read is made with a larger buffer to ensure the data is not lost.

Reference issue

Fixes #718

@codecov-io
Copy link

codecov-io commented Jun 19, 2019

Codecov Report

Merging #719 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #719      +/-   ##
==========================================
+ Coverage   76.63%   76.64%   +<.01%     
==========================================
  Files          58       58              
  Lines        4041     4038       -3     
==========================================
- Hits         3097     3095       -2     
+ Misses        688      687       -1     
  Partials      256      256
Impacted Files Coverage Δ
datachannel.go 82.24% <ø> (+0.88%) ⬆️
sctptransport.go 74.34% <0%> (-1.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2228e7f...e7386d4. Read the comment docs.

@cohosh
Copy link
Contributor Author

cohosh commented Jul 3, 2019

As mentioned in #718, I'm revising this patch to do two things:

  1. Increase the size of dataChannelBufferSize to match the send limit in WriteSCTP https://github.com/pion/sctp/blob/master/stream.go#L146
  2. Log an error instead of a warning if io.ErrShortBuffer is returned and close the datachannel

Copy link
Member

@enobufs enobufs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cohosh - I am so sorry for the delay. I was distracted by other issues...
Changes look good and reasonable!
I have just tagged the latest pion/sctp as v1.6.4. Not strictly required but if you could update go.mod before merging, that would be fantastic. (rebase master also)

go get -u github.com/pion/sctp@v1.6.4
go mod tidy

The current dataChannelBufferSize is less than the limit used in
WriteSCTP (called by Send), which means that peers using this
implementations of WebRTC can send messages that will be thrown out by
Read because of an io.ErrShortBuffer error.
In the case that the message is too long, a more appropriate response
would be to close the channel and issue an error rather than log a
warning and continue reading data.
Updated by running the following commands:
go get -u github.com/pion/sctp@v1.6.4
go mod tidy
@cohosh cohosh merged commit d77af77 into pion:master Jul 12, 2019
@cohosh
Copy link
Contributor Author

cohosh commented Jul 12, 2019

Rebased and merged. The last commit updated the go module with the commands you gave. Thanks @enobufs!

@Sean-Der
Copy link
Member

Thanks again for getting involved @cohosh

If there is anything we can ever do to help I am ears. Really excited/grateful to have people actually using Pion :)

@AeroNotix
Copy link
Contributor

AeroNotix commented Aug 28, 2019

There are various issues/PRs/comments where this is brought up, however, I ran into an issue in some code I am writing where I detach the data channel for use in another protocol on top of the detached datachannel - the protocol's library does a tonne of reads with a buffer with length 1.

Is the idea that if you detach the datachannel, you are supposed to handle these use cases on your own?

If detaching the datachannel is supposed to act like an io.Reader, for example - requesting that the buffer be made larger to accommodate the io.Read call seems strange to me.

@cohosh
Copy link
Contributor Author

cohosh commented Oct 3, 2019

Just to give an update: the latest version of Tor Browser alpha finally has Snowflake available for Windows thanks to switching to the pion/webrtc library https://blog.torproject.org/new-release-tor-browser-90a7

Thanks for all your help with this! \o/

@Sean-Der
Copy link
Member

Sean-Der commented Oct 3, 2019

That if fantastic news @cohosh !

Thank you so much for taking a risk on us, and I am always here to help whenever things come up! Using Pion for Snowflake was one of the original goals for me, so that really makes my day.

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

io.ErrShortBuffer not handled in datachannel readLoop()
5 participants