From 8cc7c19a382ea44163ea9f33f4c3c828d7630305 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Johannes=20W=C3=BCrbach?= <johannes.wuerbach@googlemail.com>
Date: Fri, 3 Jun 2022 09:01:14 +0200
Subject: [PATCH] Revert "fix: lock less when syncing (#988)"

This reverts commit 756b994db3cff63585de139e173a847e4ceeb0ba.
---
 enforcer.go             | 73 +++++++++++++++++++----------------------
 enforcer_synced.go      | 16 +--------
 enforcer_synced_test.go |  9 -----
 go.mod                  |  2 +-
 4 files changed, 36 insertions(+), 64 deletions(-)

diff --git a/enforcer.go b/enforcer.go
index 96da6ab1f..6d4941949 100644
--- a/enforcer.go
+++ b/enforcer.go
@@ -287,37 +287,46 @@ func (e *Enforcer) ClearPolicy() {
 
 // LoadPolicy reloads the policy from file/database.
 func (e *Enforcer) LoadPolicy() error {
-	newModel, err := e.prepareNewModel(e.model.Copy())
-	if err != nil {
-		return err
-	}
+	needToRebuild := false
+	newModel := e.model.Copy()
+	newModel.ClearPolicy()
 
-	if e.autoBuildRoleLinks {
-		if err := e.tryBuildingRoleLinksWithModel(newModel); err != nil {
-			return err
+	var err error
+	defer func() {
+		if err != nil {
+			if e.autoBuildRoleLinks && needToRebuild {
+				_ = e.BuildRoleLinks()
+			}
 		}
-	}
-	e.model = newModel
-	return nil
-}
-
-// prepareNewModel prepares a new model with the policy from file/database.
-func (e *Enforcer) prepareNewModel(newModel model.Model) (model.Model, error) {
-	newModel.ClearPolicy()
+	}()
 
-	if err := e.adapter.LoadPolicy(newModel); err != nil && err.Error() != "invalid file path, file path cannot be empty" {
-		return nil, err
+	if err = e.adapter.LoadPolicy(newModel); err != nil && err.Error() != "invalid file path, file path cannot be empty" {
+		return err
 	}
 
-	if err := newModel.SortPoliciesBySubjectHierarchy(); err != nil {
-		return nil, err
+	if err = newModel.SortPoliciesBySubjectHierarchy(); err != nil {
+		return err
 	}
 
-	if err := newModel.SortPoliciesByPriority(); err != nil {
-		return nil, err
+	if err = newModel.SortPoliciesByPriority(); err != nil {
+		return err
 	}
 
-	return newModel, nil
+	if e.autoBuildRoleLinks {
+		needToRebuild = true
+		for _, rm := range e.rmMap {
+			err := rm.Clear()
+			if err != nil {
+				return err
+			}
+		}
+		err = newModel.BuildRoleLinks(e.rmMap)
+		if err != nil {
+			return err
+		}
+	}
+	e.model = newModel
+	return nil
 }
 
 func (e *Enforcer) loadFilteredPolicy(filter interface{}) error {
@@ -435,8 +444,8 @@ func (e *Enforcer) EnableAutoBuildRoleLinks(autoBuildRoleLinks bool) {
 	e.autoBuildRoleLinks = autoBuildRoleLinks
 }
 
-// buildingRoleLinksWithModel attempts to build the role links for a new model
-func (e *Enforcer) buildingRoleLinksWithModel(newModel model.Model) error {
+// BuildRoleLinks manually rebuild the role inheritance relations.
+func (e *Enforcer) BuildRoleLinks() error {
 	for _, rm := range e.rmMap {
 		err := rm.Clear()
 		if err != nil {
@@ -444,21 +453,7 @@ func (e *Enforcer) buildingRoleLinksWithModel(newModel model.Model) error {
 		}
 	}
 
-	return newModel.BuildRoleLinks(e.rmMap)
-}
-
-// BuildRoleLinks manually rebuild the role inheritance relations.
-func (e *Enforcer) BuildRoleLinks() error {
-	return e.buildingRoleLinksWithModel(e.model)
-}
-
-// tryBuildingRoleLinksWithModel attempts to build the role links for a new model falls back to the existing model in case of errors
-func (e *Enforcer) tryBuildingRoleLinksWithModel(newModel model.Model) error {
-	if err := e.buildingRoleLinksWithModel(newModel); err != nil {
-		_ = e.buildingRoleLinksWithModel(e.model)
-		return err
-	}
-	return nil
+	return e.model.BuildRoleLinks(e.rmMap)
 }
 
 // BuildIncrementalRoleLinks provides incremental build the role inheritance relations.
diff --git a/enforcer_synced.go b/enforcer_synced.go
index a252ce317..8a3949c5c 100644
--- a/enforcer_synced.go
+++ b/enforcer_synced.go
@@ -110,23 +110,9 @@ func (e *SyncedEnforcer) ClearPolicy() {
 
 // LoadPolicy reloads the policy from file/database.
 func (e *SyncedEnforcer) LoadPolicy() error {
-	e.m.RLock()
-	cleanedNewModel := e.model.Copy()
-	e.m.RUnlock()
-	newModel, err := e.prepareNewModel(cleanedNewModel)
-	if err != nil {
-		return err
-	}
-
 	e.m.Lock()
 	defer e.m.Unlock()
-	if e.autoBuildRoleLinks {
-		if err := e.tryBuildingRoleLinksWithModel(newModel); err != nil {
-			return err
-		}
-	}
-	e.model = newModel
-	return nil
+	return e.Enforcer.LoadPolicy()
 }
 
 // LoadFilteredPolicy reloads a filtered policy from file/database.
diff --git a/enforcer_synced_test.go b/enforcer_synced_test.go
index 9576632a3..4377869b9 100644
--- a/enforcer_synced_test.go
+++ b/enforcer_synced_test.go
@@ -40,15 +40,6 @@ func TestSync(t *testing.T) {
 	testEnforceSync(t, e, "bob", "data2", "read", false)
 	testEnforceSync(t, e, "bob", "data2", "write", true)
 
-	// Simulate a policy change
-	e.ClearPolicy()
-	testEnforceSync(t, e, "bob", "data2", "write", false)
-
-	// Wait for at least one sync
-	time.Sleep(time.Millisecond * 300)
-
-	testEnforceSync(t, e, "bob", "data2", "write", true)
-
 	// Stop the reloading policy periodically.
 	e.StopAutoLoadPolicy()
 }
diff --git a/go.mod b/go.mod
index 62fe53671..5df294942 100644
--- a/go.mod
+++ b/go.mod
@@ -5,4 +5,4 @@ require (
 	github.com/golang/mock v1.4.4
 )
 
-go 1.13
+go 1.13
\ No newline at end of file