I have this registry struct that has a channel to unregister clients. Basically deletes the client pointer from a map and closes the client send channel. Why would this test keep failing
func (r *SocketRegistry) run() {
for {
select {
case client := <-r.Register:
r.clients[client.id] = client
case client := <-r.UnRegister:
delete(r.clients, client.id)
close(client.send)
case payload := <-r.Broadcast:
var regPayload RegistryPayload
json.Unmarshal(payload, ®Payload)
client := r.clients[regPayload.ClientID]
select {
case client.send <- payload:
default:
close(client.send)
delete(r.clients, client.id)
}
}
}
}
//GetClient returns the Client pointer by id
func (r *SocketRegistry) GetClient(id string) (*Client, bool) {
if client, ok := r.clients[id]; ok {
return client, ok
}
return &Client{}, false
}
Here is the Test
func TestRegisterClient(t *testing.T) {
registry := Registry()
go registry.run()
defer registry.stop()
var client Client
client.id = "PROPS"
client.send = make(chan []byte, 256)
registry.Register <- &client
c, _ := registry.GetClient(client.id)
if client.id != c.id {
t.Errorf("Expected client with id: %v got: %v", client.id, c.id)
}
registry.UnRegister <- &client
c, ok := registry.GetClient(client.id)
if ok {
t.Errorf("Expected false got ok: %v and client id: %v got: %v", ok, client.id, c.id)
}
}
it's as if the map never deletes the key. If I add some log statements then it does delete the key, which makes me think that maybe this is a timing issue with goroutines
There's a race. There's no guarantee that run()
executes delete(r.clients, client.id)
before registry.GetClient(client.id)
is called.
The race detector detects and reports the issue.
Implement GetClient like this:
// add this field to Registry
get chan getRequest
struct getRequest struct {
ch chan *Client
id string
}
func (r *SocketRegistry) GetClient(id string) (*Client, bool) {
ch := make(chan *Client)
r.get <- getRequest{id, ch}
c := <- ch
if c == nil {
return &Client{}, false
}
return c, true
}
func (r *SocketRegistry) run() {
for {
select {
case gr := <-r.get:
gr.ch <- r.clients[id]
case client := <-r.Register:
... as before
}
}
I'd use a mutex instead of a channels and goroutine to solve this problem:
func (r *SocketRegistry) register(c *Client) {
r.mu.Lock()
defer r.mu.Unlock()
r.clients[c.id] = c
}
func (r *SocketRegistry) unregister(c *Client) {
r.mu.Lock()
defer r.mu.Unlock()
delete(r.clients, c.id)
close(c.send)
}
func (r *SocketRegister) get(id string) (*Client, bool) {
r.mu.Lock()
defer r.mu.Unlock()
c, ok := r.clients[id]
return c, ok
}
func (r *SocketRegistry) send(id string, data []byte) {
r.mu.Lock()
defer r.mu.Unlock()
c := r.clients[id]
select {
case c.send <- data:
default:
close(c.send)
delete(r.clients, c.id)
}
}
Goroutines are awesome, but they are not always the best tool for a given job.