Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

kingzytgit
Copy link

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

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@hsluoyz
Copy link
Member

hsluoyz commented Apr 25, 2024

@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 {
Copy link
Contributor

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.

adapter.go Show resolved Hide resolved
@hsluoyz
Copy link
Member

hsluoyz commented Apr 26, 2024

@kingzyt

@kingzytgit
Copy link
Author

kingzytgit commented Apr 28, 2024

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.

@MuZhou233
Copy link
Contributor

if transactionMu is initially nil, and two Transaction instances are executed almost simultaneously, and both pass the transactionMu==nil check before either of them assigns a value to transactionMu, the second assignment will overwrite the first one. This can lead to a concurrency safety issue where not the same mutex protects both operations, but rather each mutex assumes it is protecting the operation.

That's exactly the original purpose of using muInitialize.

Keeping the nil check for Transaction seems unnecessary to me from a logical standpoint.

Sure it is not such neccessay. I'm thinking the transactionMu is newly added and there may be code somewhere didn't initialize it. A nil check could avoid potential panic. (e.g. If someone create Adapter without provided function, mutex would be nil after upgrade this lib)


The old mutex initialize logic do have concurrency safety issues. I think your solution is better.

@yuikns
Copy link
Contributor

yuikns commented May 30, 2024

I am meeting the same issue. I noticed the previous PR has been beautifully crafted and I'm really impressed with the work done.
May I know will it merged soon? @MuZhou233

@MuZhou233
Copy link
Contributor

I am meeting the same issue. I noticed the previous PR has been beautifully crafted and I'm really impressed with the work done. May I know will it merged soon? @MuZhou233

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.

@hsluoyz
Copy link
Member

hsluoyz commented May 31, 2024

Replaced by: #237

@hsluoyz hsluoyz closed this May 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants