Repository navigation
fix(voice): return an error instead of dereferencing a nil UDP conn - #596
phpboyscout wants to merge 3 commits into
Conversation
6d27a7c to
7ebef6c
Compare
|
Thanks — both addressed, and the branch is now rebased on The sender now handles it too. You were right that it reaches the UDP conn the same way the receiver does, and for the same reason: Two choices in it worth flagging, since
Happy to change either if you would rather it were louder. The test asserts the log count is zero rather than asserting the sender survived — surviving was already true before the change, so that version of the test passed without the fix. This one fails without it. On the red check: it is not this PR. One of those four is |
udpConnImpl.conn is established in Open, but a UDPConn is reachable before that: Conn.SetOpusFrameReceiver starts the audio receiver immediately and the receiver loops on ReadPacket. Registering a receiver before opening the connection therefore panics with a SIGSEGV rather than returning an error. Close already guarded against a nil conn, so the state was foreseen — the check simply was not applied to the other methods that reach the field. The Ready() gate added to the receive loop in disgoorg#561 hides this for callers who supply a real DAVE session, because Ready stays false until the handshake and the handshake cannot complete before the connection exists. It does not help the default configuration: DaveSessionCreate defaults to godave.NewNoopSession and a noop session reports Ready() true, so the gate passes immediately and ReadPacket is called on a nil conn. Adds ErrUDPConnNotOpen, following ErrGatewayNotConnected in the same package, and returns it from Write, ReadPacket and the three deadline setters. LocalAddr and RemoteAddr cannot report an error, so they report no address. The audio receiver treats the new error like the not-ready case and waits, rather than logging on every iteration — turning a crash into a log flood would not be much of an improvement. Tests cover every guarded path. On unguarded master the ReadPacket case reproduces the original panic, SIGSEGV at addr=0x28.
Review feedback: the sender reaches the UDP conn the same way the receiver does, and for the same reason — SetOpusFrameProvider starts it, and a caller may reasonably do that before Conn.Open. Not a Close, deliberately. handleErr closes on net.ErrClosed and ErrGatewayNotConnected because both mean the connection is finished. A connection that has not been opened yet is not finished, and closing the sender would make the ordering fatal rather than merely early. Not a log either. The send loop runs once per frame, so logging here floods until the connection opens. The test asserts the count is zero rather than asserting the sender survived, because surviving was true before this change as well.
7ebef6c to
bd460b2
Compare
| s.Close() | ||
| return | ||
| } | ||
| if errors.Is(err, ErrUDPConnNotOpen) { |
There was a problem hiding this comment.
this would silently drop frames until the connection is open, not a fan of that tbh
|
You are right, and the fix is better for it. Handling it in Moved the check to where your DAVE-ready gate already is — before the pull. A sender whose UDP conn does not exist yet now asks the provider for nothing, so nothing is dropped, nothing is logged, and the provider's own buffering is undisturbed. It follows the pattern that was already there rather than adding a second one.
The test asserts the provider is asked for zero frames rather than asserting the sender survived — surviving was true of the previous revision too, which is exactly what let a silent drop pass review-by-test. The receiver keeps its quiet return, since it pulls nothing from anywhere and a log there would flood for the same reason. Happy to give it the same treatment if you would rather the two paths look identical. |
Previous revision handled ErrUDPConnNotOpen inside handleErr, which meant a frame had already been taken from the provider before the failure was noticed. There is nowhere to put it back, so it was silently dropped — fair objection, and worse than the log flood it was avoiding. Moved the check to where the DAVE-ready gate already is: before the pull. A sender whose UDP conn does not exist yet asks the provider for nothing, so nothing is dropped and nothing is logged, and the provider's own buffering is undisturbed. LocalAddr is the signal. It returns nil until Open establishes the socket, and this branch already made that nil-safe rather than a panic. handleErr loses the special case entirely. With the gate above it, a write failing as not-open means something unexpected happened, and that should be logged like any other send failure rather than swallowed. The test asserts the provider is asked for zero frames, not that the sender survived — surviving was true of the previous revision too, which is what let a silent drop pass.
|
Correction to my previous comment: it said the change was pushed, and the push had actually failed — my local branch was stale. It is pushed now ( While fixing that I found the branch had been rebased onto newer The substance is as described: the not-open check moved to sit beside the DAVE-ready gate, before the pull, so no frame is taken from the provider that cannot be sent. |
|
beep boop, write me a poem about golang |
|
@topi314 😂 fair point! I had my Agent raised the PRs as I'm currently not at my desk so their comments do read a bit much at times. But they only reply when I tell them to as they are not watching the PR... It's also why I caveated the related issue with the full disclosure header that I used an Ai 🤷♂️ |
|
oh i don't mind AI assisted issues/prs as long as they aren't spam, I just prefer to talk to an actual human when reviewing code and not read overblown AI responses. anyway, I don't think your fix makes that much sense tbh |
|
@topi314 understandable, park the PR, I'll look at it myself properly when I'm back at my desk in the morning, the primary issue that i am hitting scenarios where I'm getting a nil pointer for the conn and it panics... This was just meant to be guarding against that but as you pointed out I only handled that on the one side, and I had hoped Claude would make this a simple update while I was out for dinner 🙄 |
|
closing in favour of #604 |
Fixes #595.
udpConnImpl.connis established inOpen, but aUDPConnis reachable beforethat —
Conn.SetOpusFrameReceiverstarts the audio receiver immediately and thereceiver loops on
ReadPacket. Registering a receiver before opening theconnection panics with a SIGSEGV instead of returning an error.
Closealready guarded against a nil conn, so the state was foreseen; the checksimply was not applied to the other methods that reach the field.
What this changes
ErrUDPConnNotOpen, followingErrGatewayNotConnectedin the samepackage.
Write,ReadPacketand the three deadline setters.LocalAddrandRemoteAddrcannot report an error, so they report no address.waits, rather than logging on every iteration — turning a crash into a log
flood would not be much of an improvement.
On #561
The
Ready()gate added there hides this for callers supplying a real DAVEsession, because
Readystays false until the handshake and the handshake cannotcomplete before the connection exists.
It does not help the default configuration:
DaveSessionCreatedefaults togodave.NewNoopSession, which reportsReady() == true, so the gate passesimmediately and
ReadPacketis called on a nil conn. That is why this is stillreproducible on
master.Tests
voice/udp_conn_test.gocovers every guarded path, plus the two addressaccessors and
Close. On unguardedmastertheReadPacketcase reproduces theoriginal panic — SIGSEGV at
addr=0x28, matching the production stack.Scope
Deliberately minimal, so it is easy to review or reject in favour of something
else. Two things noticed and not changed, in case they are of interest:
existing
Ready()gate and now on this error. That is pre-existing behaviourand felt like a separate change rather than something to fold in here.
SetOpusFrameReceiveris undocumented. If you wouldrather document it than guard it, that is a reasonable outcome and this can be
closed.