-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: wrong type of locking #1061
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@tangyang9464 please review |
7beac0e
to
16cb80e
Compare
@@ -131,8 +131,8 @@ func (e *SyncedEnforcer) LoadIncrementalFilteredPolicy(filter interface{}) error | |||
|
|||
// SavePolicy saves the current policy (usually after changed with Casbin API) back to file/database. | |||
func (e *SyncedEnforcer) SavePolicy() error { | |||
e.m.RLock() | |||
defer e.m.RUnlock() | |||
e.m.Lock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will there be a race in SavePolicy, I think it just reads the resource
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Saving policies cause the adapter to write to, i.e., a database.
A RemovePolicy()
and the SavePolicy
will emit concurrent database writes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The adapter itself should ensure transaction security, but it seems that it does not now, it can only be locked here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
🎉 This PR is included in version 2.51.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
SavePolicy()
should require the lock for RW.