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

No reliable way to be notified of a Stream buffer becoming empty #316

Open
rob-deutsch opened this issue Mar 13, 2024 · 3 comments
Open

Comments

@rob-deutsch
Copy link
Contributor

rob-deutsch commented Mar 13, 2024

Summary

It seems like there is currently no reliable way for an application to be notified when a Stream buffer has become empty.

Description

The "intuitive" way to be notified on an empty Stream buffer doesn't work because it introduces a race condition.

The "intuitive" way is to use the onBufferedAmountLow callback, but if you're using that to feed the buffer you'll have to use Stream.SetBufferedAmountLowThreshold(1) within the callback, and this is a race condition.

Its a race condition because the buffer might've been emptied between the time of the callback starting and Stream.SetBufferedAmountLowThreshold(1) being called.

Relevant code
https://github.com/pion/sctp/blob/80ec14ed0187c64a4ec29950ff029e2013be17b7/stream.go#L427C1-L432C3

Comparison to Javascript

I think this isn't an issue for the Javascript webrtc implementation because it is single threaded. The SCTP component can not process the stream while the onBufferedAmountLow callback is running.

Solutions

This will likely require additional features not included in the Javascript webrtc spec.

The solution should 1) be consistent with other additions and deviations that pion/webrtc has made, and 2) be consistent with the pion/webrtc API style. I am not an expert in either of these.

The simplest way to solve it would be to add a field to the Config called AtomicCallbacks and if its true call s.lock.Unlock() after the callback has been run.

Drawbacks of this approach are: 1) This functionality would need to be bubbled up to datachannel.Datachannel and webrtc.Datachannel, and 2) This would represent "hidden state" whereby different "instances" of datachannels would behave differently.

@rob-deutsch rob-deutsch changed the title No reliable way to be notified of a Stream becoming empty No reliable way to be notified of a Stream buffer becoming empty Mar 13, 2024
@rob-deutsch
Copy link
Contributor Author

I have read #314 and #77.

When blocking calls are implemented this "issue" will move to the pion/datachannel package (or pion/webrtc package) where they try to replicate Javascript functionality.

@edaniels
Copy link
Member

@rob-deutsch I think when we do blocking calls, it will bubble up the blocking to datachannels as well, both breakaways from the JS functionality. Would that work?

@rob-deutsch
Copy link
Contributor Author

rob-deutsch commented Mar 14, 2024

I'm not an expert in pion's codebase, but I believe that would work.

To expand on my viewpoint for the future...

  1. In my opinion the pion/datachannel package should replicate the blocking functionality of this underlying pion/sctp package.
  2. I would expect that the pion/webrtc package would/should be responsibile for replicating the JS functionality ontop of the blocking pion/datachannel package.

A decision will need to be made about how to replicate the JS functionality. Specifically a decision about locking a mutex when running the dc.onBufferedAmountLow callback is running.

The implementation currently in pion/sctp only works in a single-threaded JS environment. To work in a Go environment the datachannel needs to be locked when the dc.OnBuffferedAmountLow callback is running so that no race conditions happen.

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

No branches or pull requests

2 participants