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

`Transaction` is not thread-safe #227

Closed hulb closed 7 months ago

hulb commented 7 months ago

It seems SetAdapter is not threa-safe:

https://github.com/casbin/gorm-adapter/blob/1f2965ad10a4ce97d91267ec9f51ca97f7833b84/adapter.go#L691-L692

It can be verified by this unit test:

func TestTransactionRace(t *testing.T) {
    a := initAdapter(t, "mysql", "root:@tcp(127.0.0.1:3306)/", "casbin", "casbin_rule")
    e, _ := casbin.NewEnforcer("examples/rbac_model.conf", a)

    cocurency := 2

    var g errgroup.Group
    for i := 0; i < cocurency; i++ {
        g.Go(func() error {
            return e.GetAdapter().(*Adapter).Transaction(e, func(e casbin.IEnforcer) error {
                _, err := e.AddPolicy("jack", "data1", "write")
                if err != nil {
                    return err
                }
                _, err = e.AddPolicy("jack", "data2", "write")
                if err != nil {
                    return err
                }
                return nil
            })

        })
    }
    require.NoError(t, g.Wait())
}

The test will fail by :

cannot start a transaction within another transaction
github-actions[bot] commented 7 months ago

:tada: This issue has been resolved in version 3.22.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

hulb commented 7 months ago

It seems that the test above still fails. Now there seems be some kind of dead lock issue. I think the set adapter way won't make it right https://github.com/casbin/gorm-adapter/blob/1f2965ad10a4ce97d91267ec9f51ca97f7833b84/adapter.go#L691-L692

The new-added test TestTransactionRace will fail when add up the cocurrency like to 100.

MuZhou233 commented 7 months ago

The new-added test TestTransactionRace will fail when add up the cocurrency like to 100.

That's true, I will make a new PR to increase concurrency in the test recently.

MuZhou233 commented 7 months ago

@hulb Before I moving on, I should say the easiest way is to add mutex lock for the whole Transaction function. Actually not all implements of IEnforcer are thread-safe. That means we cannot do concurrent operations safely without changing the function signature. Is that acceptable for you? Do you have other ideas?

hulb commented 7 months ago

No, I agree. There is no other better way in my mind. Thanks for asking and the hard work!