gorm-adapter icon indicating copy to clipboard operation
gorm-adapter copied to clipboard

fix: initialize transactionMu correctly and ensure the transitivity of transactionMu

Open kingzytgit opened this issue 2 months ago • 5 comments

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

kingzytgit avatar Apr 25 '24 09:04 kingzytgit

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.

CLAassistant avatar Apr 25 '24 09:04 CLAassistant

@MuZhou233 plz review

hsluoyz avatar Apr 25 '24 10:04 hsluoyz

@kingzyt

hsluoyz avatar Apr 26 '24 16:04 hsluoyz

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.

kingzytgit avatar Apr 28 '24 01:04 kingzytgit

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.

MuZhou233 avatar Apr 28 '24 02:04 MuZhou233