Race conditions #9
Conversation
| run: make integration-up | ||
|
|
||
| - name: Run integration tests | ||
| run: GO111MODULE=on go test ./... -v |
There was a problem hiding this comment.
nit: GO111MODULE=on is default as of 1.16, so can be removed. https://golang.org/doc/go1.16#go-command
| # - name: Block Concurrent Executions of Integration Tests | ||
| # if: ${{ env.NGROK_TOKEN != '' }} | ||
| # uses: softprops/turnstyle@v1 | ||
| # with: | ||
| # poll-interval-seconds: 10 | ||
| # same-branch-only: false | ||
| # env: | ||
| # GITHUB_TOKEN: ${{ github.token }} | ||
|
|
||
| # - name: Setup integration resources | ||
| # run: export NGROK_DIR=$GITHUB_WORKSPACE/bin/ && make integration-up | ||
|
|
||
| # - name: Run integration & unit tests | ||
| # run: make integration-test | ||
|
|
||
| # - name: Compile all targets | ||
| # run: make all |
There was a problem hiding this comment.
nit: I assume this can be removed.
| // TODO: Add logging for these cases | ||
| if awsErr, ok := err.(awserr.Error); ok && awsErr.Code() == conditionalFail && cp.lastUpdate < time.Now().Add(-cp.maxAgeForClientRecord).UnixNano() { | ||
|
|
||
| // TODO: investigate if not marking cp.dirty = false here causes duplicates |
| config Config // configuration struct | ||
| numberOfRuns int32 // Used to atomically make sure we only ever allow one Run() to be called | ||
| isLeader bool // Whether this client is the leader | ||
| unbecomingLeader bool // Flag to avoid race condition when unbecoming leader |
There was a problem hiding this comment.
nit: unbecomingLeader made me chuckle. I assume this isn't describing a leader which is unsuitable, or unattractive... but a leader who is stepping down from being a leader?
Does retiringLeader better describe this, or isLeaderRetiring?
There was a problem hiding this comment.
After reading the usages of this flag, I understand the naming now (due to unbecomeLeader) 🤷
| numberOfRuns int32 // Used to atomically make sure we only ever allow one Run() to be called | ||
| isLeader bool // Whether this client is the leader | ||
| unbecomingLeader bool // Flag to avoid race condition when unbecoming leader | ||
| restartingConsumers bool // Flag to indicate that we should only update the clients table while restarting consumers |
There was a problem hiding this comment.
nit: I like flags that start with is. Lets me know its a "status" flag.
| restartingConsumers bool // Flag to indicate that we should only update the clients table while restarting consumers | |
| isRestartingConsumers bool // Flag to indicate that we should only update the clients table while restarting consumers |
| // block until we know we're not unbecoming leader already | ||
| iteration := 0 | ||
| for { | ||
| if !k.unbecomingLeader { // TODO: does looping indefinitely like this have any negative effects? should we sleep for 50ms in the loop? |
There was a problem hiding this comment.
You're definitely going to be blocking a thread to some degree here, and perhaps would lead to performance hiccups in the hosting app. A sleep is a fast way to stop this being too painful. Typically this is a bit of an antipattern though, you'd want to be blocking execution using a channel or something similar, however for brevity I think plopping a sleep here isn't too bad. I don't expect we'd get stuck in this loop for particularly long or for us to end up in here that often.
There was a problem hiding this comment.
OK so just to check my understanding:
for {
}
blocks a thread
for {
time.Sleep(100)
}
also blocks a thread, but with less impact? Or makes no difference?
I don't know why I have this impression but it feels like go has more opportunity to context switch when there's a sleep but reflecting on that might not make sense.
There was a problem hiding this comment.
When you sleep in a thread, you give other threads the opportunity to do some work. So you're still blocking execution of this particular thread but your only checking your condition every x milliseconds rather than as fast as possible, which gives the cpu a little more time to do other tasks in between checks. So yes, it still blocks but with less impact.
|
Closing in favour of #13 |
Resolves the issues summarised in #3
Specific descriptions and reasoning for each change can be found in the below issues:
#6
#5
#3
Note that the integration tests do sometimes fail due to duplicates - about 1 in 5 GH actions has one of 3 tests fail with this. So about 1 in 15 runs of 4k events.
I have some ideas of how we might attempt to avoid those duplicates. I don't believe that this behaviour is caused by my changes. The previous strategy was to crash when we hit a scenario that might produce these duplicates (without mitigating the cause of duplicates). It is impossible to say conclusively however since the integration tests weren't running previously.