[client] Fix state manager race conditions #2890
Conversation
| if err != nil { | ||
| return fmt.Errorf("marshal states: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
Why do not unlock the m.mu? It blocks the Updated function calls in the worst case for 5 sec. After this point, the code does not touch the m.state so make no sense to protect it.
There was a problem hiding this comment.
We still need to clean up the dirty map further down. If we unlock here we might clear dirty with new entries that haven't been yet written.
There was a problem hiding this comment.
The log running code (5 sec) really does not cause any issue outside of the statemanager?
There was a problem hiding this comment.
It can delay updating iptables/nftables/routes/dns on network map updates. Because of routes it might also delay p2p connections
| start := time.Now() | ||
| go func() { | ||
| done <- util.WriteJsonWithRestrictedPermission(ctx, m.filePath, m.states) | ||
| done <- util.WriteBytesWithRestrictedPermission(ctx, m.filePath, bs) |
There was a problem hiding this comment.
What will be if this function running more then 10 sec? The ticker will start a PersistState call and will be a conflict in the file.
There was a problem hiding this comment.
That's what the ctx check and deadline is for in this fn
There was a problem hiding this comment.
But the check and move is not an atomic operation. If the code runs parallel this lines with two different ctx the outcome is unpredictable.
// Check context again
if ctx.Err() != nil {
return ctx.Err()
}
if err = os.Rename(tempFileName, file); err != nil {
return fmt.Errorf("move %s to %s: %w", tempFileName, file, err)
}
```
|
Describe your changes
Issue ticket number and link
Checklist