fix: Data race in sentries #5
Conversation
|
@agouin What's the best way to test this in the wild? Deploy a custom image to the sentry-testnet vcluster? It would be ideal to have a |
| done := make(chan struct{}) | ||
| cometos.TrapSignal(a.logger, func() { | ||
| for _, s := range a.sentries { |
There was a problem hiding this comment.
One part of the race is we read from the map here.
|
|
||
| if err := s.Start(); err != nil { | ||
| return fmt.Errorf("failed to start new remote signer(s): %w", err) | ||
| } | ||
| a.sentries[newSentry] = s |
There was a problem hiding this comment.
The 2nd part of the race is we are reading and mutating the sentries map here. This is in a separate goroutine. There was no guarantee this goroutine would exit while the main() goroutine needing to read the map.
| close(w.stop) | ||
| <-w.done |
There was a problem hiding this comment.
The synchronization here ensures only one goroutine is modifying w.sentries.
| return fmt.Errorf("failed to start listener(s): %w", err) | ||
| } | ||
| defer logIfErr(logger, loadBalancer.Stop) |
There was a problem hiding this comment.
Previously loadBalancer would not have been stopped if watching sentries failed.
Yes in the sentry-testnet vcluster is good, no concerns if we have uptime issues there. |
|
I'll test a prerelease on the testnet sentries |
This includes a bit of code cleanup. Passing around a pointer to a struct
*appStatelends itself to races (as in this case).As such, this PR removes
appState.This PR also ensures that we properly close all connections when the process is terminated.