Patch to properly return an io.ErrShortBuffer

[cohosh]

If the reassemblyQueue returns an io.ErrShortBuffer, this error is
returned instead of being overwritten and lost

Description

Since the datachannel read loop https://github.com/pion/webrtc/blob/master/datachannel.go#L292 is checking for io.ErrShortBuffer errors, the best thing to do is simply return this error from Stream.ReadSCTP
and let the calling functions handle it.

See https://trac.torproject.org/projects/tor/ticket/28942#comment:15

Reference issue

Fixes #50

[cohosh]

Ah, I'm seeing an issue now about how the caller should handle the error. I think it's a good idea to allow the caller to try again with a larger buffer. Right now the reassemblyQueue increments the SSN and removes the chunkset from the list. I think the reassembly queue should leave the chunkset in the list until the read returns without error.

[codecov-io]

Codecov Report

Merging #51 into master will decrease coverage by 0.09%.
The diff coverage is 81.81%.

@@            Coverage Diff            @@
##           master      #51     +/-   ##
=========================================
- Coverage   75.72%   75.63%   -0.1%     
=========================================
  Files          43       43             
  Lines        2612     2618      +6     
=========================================
+ Hits         1978     1980      +2     
- Misses        510      512      +2     
- Partials      124      126      +2

Impacted Files	Coverage Δ
stream.go	87.68% <0%> (-1.29%)	⬇️
reassembly_queue.go	97.7% <100%> (+0.05%)	⬆️
association.go	81.38% <0%> (-0.2%)	⬇️

---

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 123d075...a7fcd22. Read the comment docs.

[enobufs]

That is clearly a bug and as io.ErrShortBuffer would be the only error that is returned at this moment. I created a follow-up isssue (#52). Thanks @cohosh for reporting this and sending PR!

I missed the changes to leaving the message in the queue. Let me review it a bit later!

[enobufs]

TODO: investigate WebRTC protocol spec, and EOR (e.g. this article

[Sean-Der]

This is super exciting, thanks for the contribution @cohosh :)

I added you to the Pion organization, when the PR gets approved you will be able to merge it right away. I really hope we can make pion/webrtc work for snowflake, I would be very proud to think we helped make Tor better :) also I think moving away from Google controlled software to something owned by the community is better for everyone.

If there is anything I can do personally I am always happy to help! We have a Slack and always feel free to drop me an email sean @ pion.ly :)

[cohosh]

Thanks! It's probably a good idea to address the changes to the datachannel read loop along with this change: pion/webrtc#719
Otherwise, this change will cause an infinite loop by the caller.

[cohosh]

Also you can follow the integration to Snowflake here: https://trac.torproject.org/projects/tor/ticket/28942
Thanks for the interest, we're excited about getting this into a usable and easily buildable state :)

[backkem]

@cohosh An alternative to this can be our detach API:
https://github.com/pion/webrtc/blob/master/examples/data-channels-detach/main.go
This allows you to detach the underlying datachannel implementation, giving you direct access to the io.ReadWriter we use internally from which you can read with your own buffer of any size.
As an example, I've used this in libp2p/go-libp2p-webrtc-direct:

https://github.com/libp2p/go-libp2p-webrtc-direct/blob/b213ddc5d17985af4a77a60757cdc5956d71e6b7/transport.go#L26-L30

This enables you to read from the data channel in an idiomatic way:

https://github.com/libp2p/go-libp2p-webrtc-direct/blob/b213ddc5d17985af4a77a60757cdc5956d71e6b7/stream.go#L20-L21

[cohosh]

@cohosh An alternative to this can be our detach API:
https://github.com/pion/webrtc/blob/master/examples/data-channels-detach/main.go
This allows you to detach the underlying datachannel implementation, giving you direct access to the io.ReadWriter we use internally from which you can read with your own buffer of any size.
As an example, I've used this in libp2p/go-libp2p-webrtc-direct:

https://github.com/libp2p/go-libp2p-webrtc-direct/blob/b213ddc5d17985af4a77a60757cdc5956d71e6b7/transport.go#L26-L30

This enables you to read from the data channel in an idiomatic way:

https://github.com/libp2p/go-libp2p-webrtc-direct/blob/b213ddc5d17985af4a77a60757cdc5956d71e6b7/stream.go#L20-L21

Hi @backkem !
Thanks for the response. I took a look at the detached channels, and I think we'd still prefer to be able to use the OnMessage callback instead of implementing our own readLoop (especially since these issues seem to be actual bugs that, if fixed, would solve the issues we're having).

[cohosh]

Oh, also just to confirm... it seems detaching the channel still requires this SCTP patch otherwise the io.ErrShortBuffer won't be returned. So channel detachment is really in case you decide to handle the io.ErrShortBuffer differently than this PR: pion/webrtc#719. It would also allow us to specify a larger buffer. Is that a good summary?

[cohosh]

Hey, as mention in pion/webrtc#718, I updated this patch to remove the modifications to the reassembly_queue, but keep the io.ErrShortBuffer return.

[enobufs]

Thanks for your PR + test!! It looks good to me.

[cohosh]

Thanks!