15

I have the following piece of code:

func sendRegularHeartbeats(ctx context.Context) {
    for {
        select {
        case <-ctx.Done():
            return
        case <-time.After(1 * time.Second):
            sendHeartbeat()
        }
    }
}

This function is executed in a dedicated go-routine and sends a heartbeat-message every second. The whole process should stop immediately when the context is canceled.

Now consider the following scenario:

ctx, cancel := context.WithCancel(context.Background())
cancel()
go sendRegularHeartbeats(ctx)

This starts the heartbeat-routine with a closed context. In such a case, I don't want any heartbeats to be transmitted. So the first case block in the select should be entered immediately.

However, it seems that the order in which case blocks are evaluated is not guaranteed, and that the code sometimes sends a heartbeat message, even though the context is already canceled.

What is the correct way to implement such a behaviour?

I could add a "isContextclosed"-check in the second case, but that looks more like an ugly workaround for the problem.

Brian Tompsett - 汤莱恩
  • 5,753
  • 72
  • 57
  • 129
maja
  • 17,250
  • 17
  • 82
  • 125
  • Although the title is very similar to https://stackoverflow.com/questions/11117382/priority-in-go-select-statement-workaround, this is not a duplicate. The solution of the other question dealt with a consumer/producer problem, not a context. – maja Sep 13 '17 at 14:36
  • See point 2 in this article. http://www.tapirgames.com/blog/golang-concurrent-select-implementation , It seems that what you are experiencing is normal `select` behavior. If you want more control you might have to reimplement without context and proceed with a `consumer/producer` solution – reticentroot Sep 13 '17 at 14:40
  • 2
    The question is not identical, but the answer is: Select on ctx.Done first and send heartbeat in the default case if not canceled (also selected). Loop. Simple. You cannot force priority in select. Don't look further, it is undoable. You have to nest selects. – Volker Sep 13 '17 at 14:42
  • @Volker good point, sending to default should do the trick – reticentroot Sep 13 '17 at 14:42
  • @Volker Can you post this as an answer with a code example? I don't really see how this should work. If the context is closed during the 1-second timeout, I want to abort as well (the timeout is configurable and can be a minute or longer) – maja Sep 13 '17 at 14:51

3 Answers3

14

The accepted answer has a wrong suggestion:

func sendRegularHeartbeats(ctx context.Context) {
    ticker := time.NewTicker(time.Second)
    defer ticker.Stop()

    for {
        //first select 
        select {
        case <-ctx.Done():
            return
        default:
        }

        //second select
        select {
        case <-ctx.Done():
            return
        case <-ticker.C:
            sendHeartbeat()
        }
    }
}

This doesn't help, because of the following scenario:

  1. both channels are empty
  2. first select runs
  3. both channels get a message concurrently
  4. you are in the same probability game as if you haven't done anything in the first select

An alternative but still imperfect way is to guard against concurrent Done() events (the "wrong select") after consuming the ticker event i.e.

func sendRegularHeartbeats(ctx context.Context) {
    ticker := time.NewTicker(time.Second)
    defer ticker.Stop()

    for {            
        //select as usual
        select {
        case <-ctx.Done():
            return
        case <-ticker.C:
            //give priority to a possible concurrent Done() event non-blocking way
            select {
              case <-ctx.Done():
              return
            default:
            }
            sendHeartbeat()
        }
    }
}

Caveat: the problem with this one is that it allows for "close enough" events to be confused - e.g. even though a ticker event arrived earlier, the Done event came soon enough to preempt the heartbeat. There is no perfect solution as of now.

Balint Pato
  • 1,497
  • 1
  • 13
  • 28
  • You are right - the accepted answer won't work if `sendRegularHeartbeats` is called and - later - both channels get closed at the exact same time. However, it will work if both channels are closed before `sendRegularHeartbeats` is called - which was exactly my specific problem. – maja Jul 22 '18 at 11:57
  • In that case (of the caveat), we can park the arrival of Done() using a flag, sendHeartbeat() since we have satisfied the ticker and then process the done immediately. – Haswell Oct 25 '20 at 17:29
8

Note beforehand:

Your example will work as you intend it to, as if the context is already cancelled when sendRegularHeartbeats() is called, the case <-ctx.Done() communication will be the only one ready to proceed and therefore chosen. The other case <-time.After(1 * time.Second) will only be ready to proceed after 1 second, so it will not be chosen at first. But to explicitly handle priorities when multiple cases might be ready, read on.


Unlike the case branches of a switch statement (where the evaluation order is the order they are listed), there is no priority or any order guaranteed in the case branches of a select statement.

Quoting from Spec: Select statements:

If one or more of the communications can proceed, a single one that can proceed is chosen via a uniform pseudo-random selection. Otherwise, if there is a default case, that case is chosen. If there is no default case, the "select" statement blocks until at least one of the communications can proceed.

If more communications can proceed, one is chosen randomly. Period.

If you want to maintain priority, you have to do that yourself (manually). You may do it using multiple select statements (subsequent, not nested), listing ones with higher priority in an earlier select, also be sure to add a default branch to avoid blocking if those are not ready to proceed. Your example requires 2 select statements, first one checking <-ctx.Done() as that is the one you want higher priority for.

I also recommend using a single time.Ticker instead of calling time.After() in each iteration (time.After() also uses a time.Ticker under the hood, but it doesn't reuse it just "throws it away" and creates a new one on the next call).

Here's an example implementation:

func sendRegularHeartbeats(ctx context.Context) {
    ticker := time.NewTicker(time.Second)
    defer ticker.Stop()

    for {
        select {
        case <-ctx.Done():
            return
        default:
        }

        select {
        case <-ctx.Done():
            return
        case <-ticker.C:
            sendHeartbeat()
        }
    }
}

This will send no heartbeat if the context is already cancelled when sendRegularHeartbeats() is called, as you can check / verify it on the Go Playground.

If you delay the cancel() call for 2.5 seconds, then exactly 2 heartbeats will be sent:

ctx, cancel := context.WithCancel(context.Background())
go sendRegularHeartbeats(ctx)
time.Sleep(time.Millisecond * 2500)
cancel()
time.Sleep(time.Second * 2)

Try this one on the Go Playground.

icza
  • 389,944
  • 63
  • 907
  • 827
4

If it is absolutely critical to maintain that priority of operations, you can:

  • Consume from each channel in a separate goroutine
  • Have each of those goroutines write a message to a shared third channel indicating its type
  • Have a third goroutine consume from that channel, reading the messages it receives to determine if it is a tick and should sendHeartbeat or if it is a cancel and it should exit

This way, messages received on the other channels will (probably - you can't guarantee order of execution of concurrent routines) come in on the third channel in the order they're triggered, allowing you to handle them appropriately.

However, it's worth noting that this is probably not necessary. A select does not guarantee which case will execute if multiple cases succeed simultaneously. That is probably a rare event; the cancel and ticker would both have to fire before either was handled by the select. The vast majority of the time, only one or the other will fire at any given loop iteration, so it will behave exactly as expected. If you can tolerate rare occurrences of firing one additional heartbeat after a cancellation, you're better off keeping the code you have, as it is more efficient and more readable.

Adrian
  • 42,911
  • 6
  • 107
  • 99
  • 2
    I understand the practical nature of your answer, but conceding "race conditions sometimes happen just not very often so it's okay" doesn't leave a great taste in the mouth. – dcow Jun 06 '20 at 07:07
  • @dcow I never said that, so I'm not sure what you're referring to? There's no data race here, all communications between goroutines are synchronized by channels. – Adrian Jun 08 '20 at 12:27
  • I guess I was having trouble digesting your last paragraph. Having a timer fire after it's been canceled is, strictly, _not correct_ and it's odd to me that you would design code that's probably okay even if the timer fires when it's not supposed to. I've been in large codebases like that and it begins to drive you insane. I understand that defensive coding is practical, but your answer doesn't actually demonstrate a way to do what the OP is asking for, (your _probably_ ordered channel doesn't really help anything because it just moves the problem around). – dcow Jun 08 '20 at 23:24
  • You can't guarantee order of concurrent operations. You can force them to be sequential using locking, but you still can't enforce their ordering. If you want strictly ordered operations, they should not be concurrent. – Adrian Jun 09 '20 at 13:41
  • I don't think we disagree on that point. However, the question is not about strict ordering. The question is, "if I have two channels that both have data ready to consume, how do I prioritize one channel over the other?" The question stands irrespective of which event actually happened _first_, which, as you point out, isn't often meaningful since concurrent events effectively all happen at the same time. – dcow Jun 10 '20 at 18:44