Repository navigation
fix(voice): start audio loops once the udp conn can back them - #604
phpboyscout wants to merge 2 commits into
Conversation
bfa35cf to
9126297
Compare
SetOpusFrameProvider and SetOpusFrameReceiver started their goroutine loops immediately, but the UDP conn is not dialled until the gateway's ready event and its encrypter is not built until the session description. Registering audio before Conn.Open dereferenced nil in a background goroutine and killed the process. The loops now start once both exist, so registering early is supported rather than fatal. The documented order is unchanged: Conn.Open returns only after the session description, so a setter called after it starts its loop straight away as before. Open on the loops assigns the cancel func before spawning the goroutine, and a mutex guards it, because deferring the start makes it reachable to close a loop that never ran. Open and Close also land on different goroutines once a loop is registered early. On master that pairing panics on a nil cancel func.
9126297 to
1b98946
Compare
|
Rebased onto current master. The only conflict was with disgoorg/disgo#612 in AudioSender.Close, and I've kept both changes: the nil-safe cancel, then closing the opus provider. The reproduction test still panics on master as of today. Happy to adjust anything. |
|
tested this on the branch and with current master merged in (after #588 and #634):
I did find one rare race, but I wouldn't block the merge on it. if a sender was registered before with a stub sender that records old := &trackSender{}
c := &connImpl{config: config, audioSender: old, udpOpened: true}
go func() { start.Wait(); c.startAudioOn(&c.secretKeySet) }()
go func() { start.Wait(); c.SetOpusFrameProvider(nil) }()
// old.Open() called after old.Close()a
|
Conn calls Open on a sender or receiver outside audioMu, so a replacement can close it first. Close then found no cancel func, and the loop Open started had nothing left to stop it. The default sender and receiver now record that they were closed: Open does nothing after Close, and a second Close is a no-op, so the provider or receiver is closed once even when the loop already closed itself on a closed udp conn.
|
Hey @thomas-vilte Thanks for testing this so thoroughly, and for the repro. I’ve added a second commit on top with the closed flag as you suggested: Close sets it under s.mu, and Open returns without starting the loop once it’s set, so nothing user-provided runs under audioMu. The same flag makes a second Close a no-op. The sender and receiver both close themselves on a closed UDP conn, so before this the conn could close the provider or receiver a second time afterwards. Two new tests on the real sender and receiver: Open after Close doesn’t start the loop, and closing twice closes the provider and receiver once. I broke the flag six ways (each check dropped, in the sender and the receiver) and the tests caught all six. go test -race ./... passes on the branch and with current master merged in, and golangci-lint is clean. This covers the default sender and receiver only. A custom AudioSender or AudioReceiver can still be opened after Close in that window, so it needs the same guard. Happy to say so in their doc comments if you think it’s worth it. Sounds good on Conn.Close(); with closeAudio() in place it should be the one-liner you describe. |
|
thanks, retested with the new commit. both new tests fail without the flag and pass with it. my double close and nil provider tests still behave as expected: provider closed once, panic stays in the caller. and yeah, a line in the |
|
this needs to be rebased now |
Fixes #595
@topi314 Following our conversation yesterday I went digging into this more and I identified something interesting when trying to make sense of my AI agents mess of #596.
The original issue was that I originally had a setup I builds the audio pipeline before it connects:
That paniced on a nil pointer and took the process with it. The docs show doing the open first, but I didnt actually see that till this morning, and (at least for me personally) this way round seemed more intuitive. But this is what the original PR was meant to address, the handling of that nil pointer if you order things in the way I do above. (There is probably an easier version of this PR that just involves documentation and something to enforce ordering but I didnt want to implement a contract change and trigger a major release)
Both setters start their loop the moment you call them, but that loop reads and writes the UDP conn, and the conn isn't dialled until the gateway's ready event. The encrypter isn't there either until the session description lands. So if you register audio before
Conn.Openthen the loop spends the whole handshake running against nils.I did wonder whether the
DAVE().Ready()check at the top of both loops alreadycovered it. It doesn't! The default is
godave.NewNoopSession, whoseReady()returns true unconditionally, so that gate is open on every default setup.
The solution
Instead of guarding the dereference (as in #596 ) and returning an error. This tries to address the allow looser ordering instead. The loops start start conn can actually back them, so registering early works instead of panicing, and there's no error to handle because the state that caused it doesn't arise.
The documented order is untouched.
Conn.Openonly returns after the session description, so a setter called after it starts its loop straight away, exactly as now.TestAudioLoopsStartWhenConnIsReadypasses on master as well as on this branch, which is what pins it.Two numbers:
master.TestRealAudioLoopsDoNotRunBeforeOpenreproduces it against the real types.SetOpusFrameProviderracing the handshake, over 300,000 test runs,the current
masterstarts a loop against a conn that can't back it 3,844 times. My proposed solution in this PR results in zeroTo make this work for both audio Sender and Reciever,
Open()now creates the cancel func before it spawns the goroutine rather than inside it, and introduced a small single purpose mutex. Without it, Open racing Close panics on a nil cancel func on master, and with just a nil check it still races. -race catches that within a couple of hundred iterations.It isn't just a case of me tidying up either. Once the loop has started, if later than the setter that registered it, you can end up closing one that never actually ran, and on
masterthat calls a nil func and panics. The two calls also end up on different goroutines: the voice gateway is what starts a loop registered early, while the caller is what closes it. So the field needed guarding either way. Without the mutex,-racepicks it up within a couple of hundred iterations.What it doesn't do
I kept this deliberately narrow. These are all untouched and behave exactly as they do on
mastertoday:Happy to look at any of those separately if you want them, but they each felt like a different change to me. Feel free to close/reject #596