What is the right way to safely finish goroutines in my code?

Question

I am writing a simple tcp server, the goroutine model is very straight forward:

One goroutine is responsible for accepting new connections; for every new connection, three goroutines are started :

1. one for read
2. one for process and handle application logic
3. one for write

Currently one server will serve no more than 1000 users, so I don't try to limit goroutine numbers.

for {
    conn, err := listener.Accept()
    // ....
    connHandler := connHandler{
        conn:      conn,
        done:      make(chan struct{}),
        readChan:  make(chan string, 100),
        writeChan: make(chan string, 100),
    }
    // ....
    go connHandler.readAll()    
    go connHandler.processAll() 
    go connHandler.writeAll()   
}

I use the done channel to notify all three channels to finish, when user logout or a permanent network error happened, done channel will be closed (use sync.Once to make sure closing only happen once):

func (connHandler *connHandler) Close() {
    connHandler.doOnce.Do(func() {
        connHandler.isClosed = true
        close(connHandler.done)
    })
}

Below is the code of writeAll() method:

func (connHandler *connHandler) writeAll() {
    writer := bufio.NewWriter(connHandler.conn)

    for {
        select {
        case <-connHandler.done:
            connHandler.conn.Close()
            return
        case msg := <-connHandler.writeChan:
            connHandler.writeOne(msg, writer)
        }
    }
}

There is a Send method to send message to a user, by sending strings to the write channel:

func (connHandler *connHandler) Send(msg string) {
    case connHandler.writeChan <- msg:
}

The Send method will be called mainly in processAll() goroutine, but also in many other goroutines because different users need to communicate with each other.

Now is the problem: if userA logout or network failed, userB send a message to userA, userB's goroutine may be permanently blocked because no one will ever receive the message from the channel.

My solution:

My first thought is to use a boolean value to make sure connHanler is not closed when sending to it:

func (connHandler *connHandler) Send(msg string) {
    if !connHandler.isClosed {
        connHandler.writeChan <- msg
    }
}

But I think connHandler.writeChan <- msg and close(done) can still happen at the same time, the possibility of blocking still exist. So I have to add a timeout:

func (connHandler *connHandler) Send(msg string) {
    if !connHandler.isClosed {
        timer := time.NewTimer(10 * time.Second)
        defer timer.Stop()
        select {
        case connHandler.writeChan <- msg:
        case <-timer.C:
            log.Warning(connHandler.Addr() + " send msg timeout:" + msg)
        }
    }

}

Now I feel the code is safe, but also ugly, and start a timer every time when sending a message feels like an unnecessary overhead.

Then I read this article: https://go101.org/article/channel-closing.html, My problem looks like the second example in the article:

One receiver, N senders, the receiver says "please stop sending more" by closing an additional signal channel

But I think this solution can't eliminate the possibility of blocking in my circumstance.

Maybe the easiest solution is to just close the write channel and let the Send method panic, then use recover to handle the panic? But this looks like an ugly way, too.

So is there a simple and straight forward way to accomplish what I want to do?

(My English is not good, so if there is any ambiguity, please point out, thanks.)

Answer 1

Your example looks pretty good, and I think you've got 90% of what you need.

I think the problem that you are seeing is with sending, when you might actually be "done".

You can use the "done" channel to notify to all the go routines that you've finished. You will always be able to read a value from a closed channel (it will be the zero value). This means that you can update your Send(msg) method to consider the done channel.

func (connHandler *connHandler) Send(msg string) {
    select {
    case connHandler.writeChan <- msg:
    case <- connHandler.done:
        log.Debug("connHandler is done, exiting Send without sending.")
    case <-time.After(10 * time.Second):
        log.Warning(connHandler.Addr() + " send msg timeout:" + msg)
    }
}

What will happen in this select now is one of:

1. msg is sent to writeChan
2. close(done) has been called elsewhere, the done chan is closed. You will be able to read from done, breaking the select.
3. time.After(...) will get to 10seconds, and you will be able to timeout the send.