Fix/ping interval panic #192
Conversation
I believe the client should be the one to close the channel since for friendly feud we can have a client session without it necessarily being connected to a hub. If we are going to close this channel (which isn't necessary in go), I think it should happening in client.go
…yers We shouldn't send to a go channel in a goroutine that isn't the reciever of that channel since it can cause an instance where that channel is closed and we have a gorountine still wanting to send to the channel. I.E client.writePump (owns channel should be able to close it) playerClient.pingInterval (outside and has chances to cause panic if sends to closed channel) I am opting to NOT close the client.send channel manually since I believe when the client eventually disconnects and the room Hub also disconnects the go-lang garbage collector will close the channel when needed.
Test Results27 tests 27 ✅ 54s ⏱️ Results for commit 723555a. |
There was a problem hiding this comment.
Great work on fixing the panic! Making the networking less complex is probably better to make the game more robust.
An alternative approach (if latency compensation becomes important again), which I think would work as well, could have kept the pingInterval but used a select statement with a default case within its loop
This should avoid the panic without completely removing the feature:
func (p *RegisteredClient) pingInterval() error {
log.Println("Started ping for interval", p.id)
ticker := time.NewTicker(5 * time.Second)
defer func() {
ticker.Stop()
}()
for {
select {
case <-ticker.C:
player, ok := p.room.Game.RegisteredPlayers[p.id]
if !ok {
log.Println("Player not found stopping ping", p.id)
return nil // Or, better, signal an error/unregister
}
player.Start = time.Now()
message, err := NewSendPing(p.id)
if err != nil {
return fmt.Errorf(" %w", err)
}
// Attempt to send, but don't block if the channel is closed.
select {
case p.client.send <- message: // Try to send
case <-p.stopPing: // Handle stopPing here
log.Println("Stop ping via channel (during send)", p.id)
return nil
default: // Execute immediately if neither above is ready
log.Println("Client send channel full or closed, skipping ping", p.id)
// Consider unregistering the client here, as it's likely disconnected.
// p.room.Hub.unregister <- p.client
}
case <-p.stopPing:
log.Println("Stop ping via channel", p.id)
return nil
}
}
}
@karlromets I tried giving this a test and I don't think it would save us on a closed channel? It seems like In our case if we were to do Only problem is that in What do you think? Apparently possible to get the extra goroutine to sync up with eachother if they are in a WaitGroup |
|
After talking to a co-worker who works in go-lang one pattern I like was passing down a |
|
Yeah my original approach was incorrect 😅
Even without I tried reproducing the issue/panic on goroutines-test. I tried delays and abrupt closures, making me think this is a super subtle timing issue, or maybe something specific to how Also tried opening 8 different browser tabs, but it seems that it's much difficult to reproduce on localhost... Also context might be relevant |
|
Closing in favor of #193 |
e2e-uimake targetFixes: #191