Adapter.SavePolicy() not safe for concurrent use?

[aavshr]

https://github.com/casbin/xorm-adapter/blob/9b17d80119cb29c6d09806fbe3dc97fcbec3c01e/adapter.go#L226-L249

Is SavePolicy() is safe for concurrent use as it drops the table and recreates it from the model? What if a go routine is in the middle of creating a table and another one tries to drop it? Here's a snippet of my logs from postgres which I think stems from calling SavePolicy() concurrently.

The error I'm getting: ERROR: relation "casbin_rule" does not exist.

I can get around this issue by having my own locking logic if that's the case.

[hsluoyz]

I think we should add a lock for:

    a.dropTable() 
    a.createTable() 

Can you make a PR to fix it?

[aavshr]

Yes, I can try working on it. I don't know if that exactly is the issue but will try to submit a PR.

[linxing]

Yes, I can try working on it. I don't know if that exactly is the issue but will try to submit a PR.

is this fixed ur issues ? https://github.com/casbin/xorm-adapter/pull/27

[hsluoyz]

@linxing thanks for the PR!

[aavshr]

From the code it seems like it's not been fixed yet. A routine should only release the lock when it's done with the entire savePolicy(). 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.

[linxing]

From the code it seems like it's not been fixed yet. A routine should only release the lock when it's done with the entire savePolicy(). 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.

just lock the entire SavePolicy func should be ok with the issuse