search

casbin / gorm-adapter

GORM adapter for Casbin, see extended version of GORM Adapter Ex at: https://github.com/casbin/gorm-adapter-ex
https://github.com/casbin/casbin
Apache License 2.0
692 stars 206 forks source link

fix: initialize transactionMu correctly and ensure the transitivity of transactionMu #236

Closed kingzytgit closed 5 months ago

kingzytgit commented 6 months ago

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 commented 6 months ago

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 commented 6 months ago

@MuZhou233 plz review

hsluoyz commented 6 months ago

@kingzyt

kingzytgit commented 6 months ago

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 commented 6 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Replaced by: https://github.com/casbin/gorm-adapter/pull/237