Concurrency and locking in a chat server

I've never done concurrent programming before, but I've written one using go channels and RWMutex, but I'm not sure if my approach is idiomatic. I feel like it could all use channels (Maybe).

The protocol is simple: Messages are delimited by '\n'. The first message client sends is it's name. The rest are normal messages. I used a map because it's add + delete is faster, and because I'm not concerned about the ordering.
I'm mostly concerned with the locks at the last 4 methods. Could I use channels here instead? Am I using locks correctly?

```
type message struct {
message string
client *client
}

type client struct {
conn net.Conn
reader *bufio.Reader
name string
}

type Server struct {
l net.Listener
clients map[*client]struct{}
messageChan chan message
port uint16
name string
m sync.RWMutex
}

// NewServer starts listening, and returns an initialized server. If an error occured while listening started, (nil, error) is returned.
func NewServer(name string, port uint16) (*Server, error) {
p := strconv.FormatUint(uint64(port), 10)
l, e := net.Listen("tcp", "localhost:"+p)
if e != nil {
return nil, e
}
return &Server{l, make(map[*client]struct{}), make(chan message), port, name, sync.RWMutex{}}, nil
}

// Start starts accepting clients + managing messages.
func (r *Server) Start() {
go r.sendMessages()
for {
conn, e := r.l.Accept()
if e != nil {
continue
}
go r.handleConn(conn)
}
}

func (r *Server) handleConn(conn net.Conn) {
reader := bufio.NewReader(conn)
name, e := reader.ReadString('\n')
name = strings.TrimSpace(name)
if e != nil {
conn.Close()
return
}

c := &client{conn, reader, name}
r.addClient(c)
r.publishMessage(c.name + " has joined the server\n")
r.handleClient(c)
}

func (r Server) handleClient(c client) {
for {
msg, e

This is a nice project to learn concurrency on, and you've already identified a few issues that concern you, and you're right in some ways. Go documentation recommends using channels when possible, over mutexes. The documentation in the sync package says: "Other than the Once and WaitGroup types, most are intended for use by low-level library routines. Higher-level synchronization is better done via channels and communication."

Channels are your friend in Go, and you should use them liberally (well, more liberally than mutexes).

There are some special cases with channels, though, and your code has a good example of how they can be used in a broken way. Your concurrency is not nearly as significant as you think it is. I will focus on this code for the moment, and we'll simplify it down, and then get back to the concurrency problem in a bit...

Client handler

This code....

func (r *Server) handleConn(conn net.Conn) {
    reader := bufio.NewReader(conn)
    name, e := reader.ReadString('\n')
    name = strings.TrimSpace(name)
    if e != nil {
        conn.Close()
        return
    }

    c := &client{conn, reader, name}
    r.addClient(c)
    r.publishMessage(c.name + " has joined the server\n")
    r.handleClient(c)
}

func (r *Server) handleClient(c *client) {
    for {
        msg, e := c.reader.ReadString('\n')
        if e == io.EOF {
            c.conn.Close()
            r.deleteClient(c)
            r.publishMessage(c.name + " has left the server\n")
            return
        } else if e != nil {
            continue
        }
        r.messageChan <- message{c.name + ": " + msg, c}
    }
}

... takes a client network connection, pulls the name, and then establishes a loop for reading and distributing messages. It distributes the messages by pushing them on to the r.messageChan channel. Let's simplify that code a whole bunch, and point out some problems as we go.

First up, you need to learn the defer statement, and use it for closing the client connection. If you change the first line of the function to be:

func (r *Server) handleConn(conn net.Conn) {
    defer conn.Close()

now you don't need to worry about closing it in a number of other places. Just returning from the function is enough to close it.

Secondly, you should use a scanner... not a Reader. See: Scanner which has the documentation "Scanner provides a convenient interface for reading data such as a file of newline-delimited lines of text." Scanner defaults to using line-termination (which is what you have) so it's easy to set up. Your code:

reader := bufio.NewReader(conn)

becomes:

scanner := bufio.NewScanner(conn)

Note that Scanner never returns an EOF error, it just returns a false scan at EOF (or any other error). The advantage for you of a scanner is that it simplifies your loops, and error handling. This is not obvious immediately in your case as you scan the name, but will become apparent when processing messages. To get the name, you need:

// find the next end-of-line.. (or perhaps EOF)
if !scanner.Scan() {
    // could not scan anything on the first scan... odd... no name.
    // the scanner.Err() may have a value if there's a problem, but we don't care.
    return
}
name := strings.TrimSpace(scanner.Text())

OK, that's how you get the name.... and now we can create the client (using the scanner instead of reader) and we also use a defer function to remove the client instead of shifting that logic in to some other place (keeping the clean-up code in the same place as the mess-up code is important for maintainability). Also, we should relocate the "publish" methods in to the addClient and deleteClient methods (where they really should be anyway):

c := &client{conn, scanner, name}
r.addClient(c)
defer r.deleteClient(c)
r.handleClient(c)

OK, so now we have a client that can process incoming messages, let's look at that code, but using a scanner now instead of a reader, and it does not need to do the deleteClient or the related "publish" of the deletion. It also does not need to close the connection:

func (r *Server) handleClient(c *client) {
    for c.scanner.Scan() {
        msg := c.scanner.Text()
        r.messageChan <- message{c.name + ": " + msg, c}
    }
    // we could check scanner.Err() here, but we are ignoring all non-EOF errors anyway
}

Now, that looks neat... so small now that it's hardly worth having it's own function, let's pull it back in to the handleConn function (which we will rename to handleClient because I prefer client... and we'll remove the scanner from the client struct because no-one else uses it in any other place... our handleClient method is now:

```
func (r *Server) handleClient(conn net.Conn) {
defer conn.Close()

scanner := bufio.NewScanner(conn)

// read off the first line as the "name"

// find the next end-of-line.. (or perhaps EOF)
if !scanner.Scan() {
// could not sc

StackExchange Code Review Q#155465, answer score: 4