-
-
Notifications
You must be signed in to change notification settings - Fork 212
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
fix: initialize transactionMu correctly and ensure the transitivity of transactionMu #236
Conversation
|
@MuZhou233 plz review |
@@ -690,31 +692,22 @@ func (a *Adapter) AddPolicies(sec string, ptype string, rules [][]string) error | |||
|
|||
// Transaction perform a set of operations within a transaction | |||
func (a *Adapter) Transaction(e casbin.IEnforcer, fc func(casbin.IEnforcer) error, opts ...*sql.TxOptions) error { | |||
// ensure the transactionMu is initialized | |||
if a.transactionMu == nil { |
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.
It would be better to keep the nil check and return error while transactionMu is nil.
I'm glad to hear there's progress on the issue! My PR is just a suggestion, to be honest, I haven't deeply considered this bug. I hope this PR is helpful for your project. |
That's exactly the original purpose of using
Sure it is not such neccessay. I'm thinking the The old mutex initialize logic do have concurrency safety issues. I think your solution is better. |
I am meeting the same issue. I noticed the previous PR has been beautifully crafted and I'm really impressed with the work done. |
Yes, we were waiting for @kingzytgit to make some code changes as I suggested. Since a month has passed, I will draft another PR to solve this issue. |
Replaced by: #237 |
In the Transaction, there was an issue with the original code that checked if transactionMu was nil. Once entered, it couldn't exit. The problem was that Store(false) should have been Store(true). However, this approach didn't feel very reliable to me, so I removed this initialization. Instead, I added the initialization of transactionMu in each of the function to create new Adapter. Additionally, at the end of the Transaction, I continued to pass transactionMu down