Adapter.SavePolicy() not safe for concurrent use? about xorm-adapter

[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 ? #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