Add locks for exposed apis for safe concurrent use

[aavshr]

A routine should only release the lock when it's done with the entire SavePolicy() and other operations. https://github.com/casbin/xorm-adapter/blob/master/adapter.go#L258-L287

// SavePolicy saves policy to database.
func (a *Adapter) SavePolicy(model model.Model) error {
    err := a.dropTable()
    if err != nil {
        return err
    }
    err = a.createTable()
    if err != nil {
        return err
    }

    var lines []*CasbinRule

    for ptype, ast := range model["p"] {
        for _, rule := range ast.Policy {
            line := savePolicyLine(ptype, rule)
            lines = append(lines, line)
        }
    }

    for ptype, ast := range model["g"] {
        for _, rule := range ast.Policy {
            line := savePolicyLine(ptype, rule)
            lines = append(lines, line)
        }
    }

    _, err = a.engine.Insert(&lines)
    return err
}

For instance, consider the following scenario where two routines (A and B) are calling savePolicy() at the same time.

1. Routine A and B both wait on the lock on dropTable()
2. Routine A gets the lock and proceeds to drop the table.
3. Routine A releases the lock.
4. Routine B obtains the lock and proceeds to drop the table. However, the table is already dropped as Routine A dropped it earlier but has not created it yet and is waiting on the lock to be released by Routine B.

I hope this is clear enough. This is only one of many scenarios I can think of when things might be problematic.

[coveralls]

Coverage increased (+1.3%) to 68.966% when pulling 13de1159642182c3f6f514cd5ef5ca13f5d4ee07 on aavshr:fix/concurrent-save-policy into bf0e59814660c5d67f3aa0de1a9bebb86822b849 on casbin:master.

[hsluoyz]

ping @linxing for: https://github.com/casbin/xorm-adapter/pull/27

Also ping our members and contributors: @nodece @GopherJ @dovics Do you have any suggestions?

[linxing]

Why not obtain the lock in the SavePolicy() ?

[hsluoyz]

@linxing it added the lock in SavePolicy():

What did you mean by Why not obtain the lock in the SavePolicy() ??

[linxing]

I mean only add the lock in SavePolicy(). I think such as LoadPolicy() and RemoveFilteredPolicy no need to locked @hsluoyz

[linxing]

BTW. gorm-adapter have the same problem.

[hsluoyz]

@aavshr

[aavshr]

I mean only add the lock in SavePolicy(). I think such as LoadPolicy() and RemoveFilteredPolicy no need to locked @hsluoyz

To prevent race conditions. Here's my thinking behind these changes. Please, correct me if I'm wrong. Consider the following scenario where there are two simultaneous calls on SavePolicy() and then LoadPolicy() and LoadPolicy() does not have to acquire a lock.

1. SavePolicy() locks the mutex and deletes the table and is now in the middle of creating a table and re-inserting the policies from memory.
2. LoadPolicy() is called and it attemps to load the policies from the database. It is allowed to do so if it does not need to obtain the lock. However, SavePolicy() has not finished running yet. This will lead to unwanted behavior as LoadPolicy() will either find that the table does not exist (as SavePolicy() deleted it) or will not load all the policies (as SavePolicy() is not finished inserting the policies in the database)

A code like this can potentially lead to unexpected behavior without the locks.

go adapter.SavePolicy()
go adapter.LoadPolicy()

BTW. gorm-adapter have the same problem.

I haven't looked at the gorm-adapter. Also, I'm not sure if the underlying engines themselves prevent these race conditions.

All this also depends on how people use the adapter. If the adapter is not meant for concurrent use, then I think we need a disclaimer to say so. I personally had to use it on a server that checks for user permissions. And it was pretty common where I had to handle concurrent requests where one request would want to read from the database( LoadPolicy()) whereas the other wants to write to it (SavePolicy()).

I also think that dropping the table entirely is also pretty dangerous and maybe there is a better way to do all this. I would love some input regarding this.

[hsluoyz]

@aavshr @linxing I remembered we already have the synced enforcer: https://github.com/casbin/casbin/blob/master/enforcer_synced.go . The API of that enforcer is protected by locks. So we don't add any locks in the adapter layer. Users can customize their own based on their scenarios.

And sorry that I have to revert this PR: https://github.com/casbin/xorm-adapter/pull/27