fix(voice): nil-guard state.ChannelID in gateway open path#551
Conversation
|
|
||
| c.state.Token = update.Token | ||
| c.state.Endpoint = *update.Endpoint | ||
| if c.state.ChannelID == nil { |
There was a problem hiding this comment.
returning here means if the voice server update arrives before the voice state update it will never connect
There was a problem hiding this comment.
Yes, missed it
I am thinking of adding a method like tryOpenGateway that spawns the gateway open goroutine once Token, Endpoint,
and ChannelID are all populated and no other open is in flight.
I will push draft changes for discussion and care about tests
There was a problem hiding this comment.
Added tryOpenGateway implementation, which should protect from nil ChannelID
Updated PR description.
129c151 to
838ba98
Compare
The open needs Token + Endpoint (from VOICE_SERVER_UPDATE) and ChannelID (from VOICE_STATE_UPDATE). HandleVoiceServerUpdate was the sole trigger and assumed VOICE_STATE_UPDATE had already populated ChannelID, so it panicked on reconnect/resume orderings and on kick races (ChannelID nilled while the open goroutine was in flight). Introduce tryOpenGateway, called from both handlers, that spawns the open goroutine only when all three fields are populated (canOpen predicate), no open is in flight (opening flag), and the gateway is in Unconnected/Disconnected. The goroutine captures state by value and loops with a 2*maximumConnectDelay parent context: each iteration that finds state changed mid-flight re-opens with the fresh state; otherwise it exits. This handles the reconnect race where stale events trigger an open for the wrong channel while the correct VStateU/VSU pair arrive during the open. gateway.open now also returns a new ErrGatewayNoChannelID sentinel when state.ChannelID is nil instead of bare-dereferencing. Internal callers won't trigger this (conn.go's helper enforces the precondition) but it makes the failure mode loud and clear for external Gateway consumers. doReconnect treats it as terminal alongside ErrGatewayAlreadyConnected.
838ba98 to
d3a4a48
Compare
| c.stateMu.Unlock() | ||
| parentCancel() | ||
| }() | ||
| for { |
There was a problem hiding this comment.
I am not a fan of this at all, this just spams connects...
you can just call open in either voice server or voice state update
after either all data is there after all
There was a problem hiding this comment.
Simplified and dropped the loop. Both handlers now call tryOpenGateway, which just checks "all three present" and spawns a single open goroutine.
I will add a check on my side.
0a10a06 to
e9f148b
Compare
Summary
gateway.openatvoice/gateway.go:183bare-derefs*state.ChannelID. It SIGSEGVs when VOICE_SERVER_UPDATE arrives before VOICE_STATE_UPDATE (no strict ordering on reconnect/resume), or when a kick nilsChannelIDwhile a VSU goroutine is in flight.Design
Required inputs are split across two events — VSU carries
Token+Endpoint, VStateU carriesChannelID— and the order isn't guaranteed. Both handlers now call a smalltryOpenGatewayhelper that checks "all three present" and spawns one open goroutine with a value-capturedstate. Whichever event arrives second triggers the open.If both events arrive close enough to spawn two opens, the second goroutine gets
ErrGatewayAlreadyConnectedfromgateway.Openand logs once.gateway.openalso returns a newErrGatewayNoChannelIDsentinel whenstate.ChannelID == nil, as defense-in-depth for externalGatewayconsumers that bypassConn.doReconnecttreats it as terminal alongsideErrGatewayAlreadyConnected.Effect on each ordering
Follow-up to #550 (
(*udpConnImpl).Closenil-deref).