How to access a variable across all connected tcp clients in go?

I'm setting up a tcp server in a pet project I'm writing in go. I want to be able to maintain a slice of all connected clients, and then modify it whenever a new client connects or disconnects from my server.

My main mental obstacle right now is whether I should be declaring a package level slice, or just passing a slice into my handler.

My first thought was to declare my ClientList slice (I'm aware that a slice might not be my best option here, but I've decided to leave it as is for now) as a package level variable. While I think this would work, I've seen a number of posts discouraging the use of them.

My other thought was to declare ClientList as a slice in my main function, and then I pass ClientList to my HandleClient function, so whenever a client connects/disconnects I can call AddClient or RemoveClient and pass this slice in and add/remove the appropriate client.

This implementation is seen below. There are definitely other issues with the code, but I'm stuck trying to wrap my head around something that seems like it should be very simple.

type Client struct {
    Name string  
    Conn net.Conn
}

type ClientList []*Client

// Identify is used to set the name of the client
func (cl *Client) Identify() error {
// code here to set the client's name in the based on input from client
}

// This is not a threadsafe way to do this - need to use mutex/channels
func (cList *ClientList) AddClient(cl *Client) {
    *cList = append(*cList, cl)
}

func (cl *Client) HandleClient(cList *ClientList) {
    defer cl.Conn.Close()
    cList.AddClient(cl)
    err := cl.Identify()
    if err != nil {
        log.Println(err)
        return
    }
    for {
        err := cl.Conn.SetDeadline(time.Now().Add(20 * time.Second))
        if err != nil {
            log.Println(err)
            return
        }
        cl.Conn.Write([]byte("What command would you like to perform?\n"))
        netData, err := bufio.NewReader(cl.Conn).ReadString('\n')
        if err != nil {
            log.Println(err)
            return
        }
        cmd := strings.TrimSpace(string(netData))
        if cmd == "Ping" {
            cl.Ping() //sends a pong msg back to client
        } else {
            cl.Conn.Write([]byte("Unsupported command at this time\n"))
        }

    }
}

func main() {
    arguments := os.Args

    PORT := ":" + arguments[1]
    l, err := net.Listen("tcp4", PORT)
    if err != nil {
        fmt.Println(err)
        return
    }
    defer l.Close()
    fmt.Println("Listening...")

    // Create a new slice to store pointers to clients
    var cList ClientList

    for {
        c, err := l.Accept()
        if err != nil {
            log.Println(err)
            return
        }
        // Create client cl1
        cl1 := Client{Conn: c}

        // Go and handle the client
        go cl1.HandleClient(&cList)
    }
}

From my initial testing, this appears to work. I am able to print out my client list and I can see that new clients are being added, and their name is being added after Identify() is called as well.

When I run it with the -race flag, I do get data race warnings, so I know I will need a threadsafe way to handle adding clients. The same goes for removing clients when I add that in.

Are there any other issues I might be missing by passing my ClientList into HandleClient, or any benefits I would gain from declaring ClientList as a package level variable instead?

Solution

Several problems with this approach.

First, your code contains a data race: each TCP connection is served by a separate goroutine, and they all attempt to modify the slice concurrently.

You might try building your code with go build -race (or go install -race — whatever you're using), and see it crash by the enabled runtime checks.

This one is easy to fix. The most straightforward approach is to add a mutex variable into the ClientList type:

type ClientList struct {
  mu      sync.Mutex
  clients []*Client
}

…and make the type's methods hold the mutex while they're mutating the clients field, like this:

func (cList *ClientList) AddClient(cl *Client) {
  cList.mu.Lock()
  defer cList.mu.Unlock()

  cList.clients = append(cList.clients, o)
}

(If you will ever encounter the typical usage pattern of your ClientList type is to frequently call methods which only read the contained list, you may start using the sync.RWLock type instead, which allows multiple concurrent readers.)

Second, I'd split the part which "identifies" a client out of the handler function. As of now, in the handler, if the identification fails, the handler exits but the client is not delisted.

I'd say it would be better to identify it up front and only run the handler once the client is beleived to be okay.

Also it supposedly worth adding a deferred call to something like RemoveClient at the top of the handler's body so that the client is properly delisted when the handler is done with it.

IOW, I'd expect to see something like this:

func (cl *Client) HandleClient(cList *ClientList) {
    defer cl.Conn.Close()

    err := cl.Identify()
    if err != nil {
        log.Println(err)
        return
    }

    cList.AddClient(cl)
    defer cList.RemoveClient(cl)

    // ... the rest of the code
}