Repository navigation
Data tracks: establish data channels - #987
Conversation
eea8c6e to
1b8aa1a
Compare
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
This report is out of date. Scroll down for Devin Review's latest report on this PR.
🔍 Devin Review: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| } | ||
|
|
||
| func (s *dataTrackSender) send(frame dataTrackFramePackets) { | ||
| s.once.Do(func() { go s.run() }) |
There was a problem hiding this comment.
probably make a start() call? It does only once, but has to check on every send if this is already done.
There was a problem hiding this comment.
guess it can be started from the constructor too. Makes things simple.
| e.dataTrackDC.OnMessage(e.handleDataTrackPacket) | ||
| e.dataTrackDC.SetBufferedAmountLowThreshold(dataTrackBufferedAmountLowThreshold) | ||
| e.dataTrackDC.OnBufferedAmountLow(e.dataTrackSender.wake) | ||
| e.dataTrackDC.OnOpen(e.dataTrackSender.wake) |
There was a problem hiding this comment.
I made a note about starting the data track sender, this is another place it can be started I think.
|
|
||
| for { | ||
| if current := s.dc(); current != dc { | ||
| dc, inFlight = current, nil |
There was a problem hiding this comment.
this is clearing inFlight on a data channel change.
Not sure if we want to keep checking for data channel. Maybe, it can be set and updated on change or even newDataTrackSender created by engine on data channel change. Would be good to keep this simple.
There was a problem hiding this comment.
I've made some changes to data channel sender in 5bbc090 to address your comments. This clarifies that in-flight packets for a particular frame should be dropped when data channel changes, and the data channel itself is set after the sender is created. For the latter point, please let me know if this is what you had in mind.
There was a problem hiding this comment.
Looks good to me. I would add some logs for the following
- when a data channel changes in datatracksender (can log in setDC and also in run loop when that change is picked up)
- when inFlight is dropped, what/how much is dropped
- the current dropped can be
Infow(could be spammy though if somebody just keeps sending at high rate on a narrow channel)
1b8aa1a to
7fd40fd
Compare
There was a problem hiding this comment.
Devin Review found 1 new potential issue.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| s.lock.Lock() | ||
| current := s.dc | ||
| s.lock.Unlock() | ||
| if current != dc { | ||
| // A partially sent frame cannot be completed on a new channel. | ||
| dc, inFlight = current, nil | ||
| } |
There was a problem hiding this comment.
🟡 Channel replacement sends stale frames
When setDataChannel replaces the channel during an active send loop, run continues writing the current frame to the old channel. It snapshots dc only before the inner loop. Packets can reach the obsolete session or be discarded.
Prompt for agents
In datatracksender.go, dataTrackSender.run snapshots s.dc once before entering the packet-send loop. setDataChannel can replace s.dc concurrently and wake the sender, but run does not consume that wake or observe the replacement until the current inner loop exits. Preserve the intended rule that a partially sent frame cannot continue on a replacement channel by rechecking the synchronized channel pointer before each packet send. When it changes, switch channels, clear inFlight, and re-evaluate readiness before sending anything else.
Was this helpful? React with 👍 or 👎 to provide feedback.
4895448 to
b0427c6
Compare
| e.dataTrackDC.OnMessage(e.handleDataTrackPacket) | ||
| e.dataTrackDC.SetBufferedAmountLowThreshold(dataTrackBufferedAmountLowThreshold) | ||
| e.dataTrackDC.OnBufferedAmountLow(e.dataTrackSender.wake) | ||
| e.dataTrackDC.OnOpen(e.dataTrackSender.wake) |
There was a problem hiding this comment.
Maybe move the data channel configuration statements (SetBufferedAmountLowThreshold, OnBufferedAmountLow, OnOpen) into the sender.setDataChannel
b0427c6 to
d851f36
Compare
d851f36 to
c069ebd
Compare
ladvoc
changed the title
c069ebd to
d5287c0
Compare
d5287c0 to
b2cfec2
Compare
b2cfec2 to
64e0334
Compare
3 participants
Adds the
_data_trackdata channel alongside the existing lossy and reliable channels, forwards incoming packets to the room, and adds a sender that paces frames onto the channel while keeping only the freshest one. This is not integrated with anything yet.Closes BOT-541