Repository: lyraproj/hiera
Pull request #30 (2019-07-15 20:45:03)

Drop caching of resolved configuration and previous lookups
This commit ensures that the hiera configuration is resolved on each
invocation and that no values are cached between invocations.

A side effect of this is that the cache provided in the ProviderContext
now has the same short life cycle as the invocation.


Patch:
diff --git a/hieraapi/dataprovider.go b/hieraapi/dataprovider.go
index b768c3f..96d189f 100644
--- a/hieraapi/dataprovider.go
+++ b/hieraapi/dataprovider.go
@@ -3,6 +3,9 @@ package hieraapi
 import "github.com/lyraproj/pcore/px"
 
 type DataProvider interface {
-	UncheckedLookup(key Key, invocation Invocation, merge MergeStrategy) px.Value
+	// Lookup performs an lookup using this data provider
+	Lookup(key Key, invocation Invocation, merge MergeStrategy) px.Value
+
+	// FullName returns a descriptive name of the data provider. Used by the explainer
 	FullName() string
 }
diff --git a/internal/config.go b/internal/config.go
index c317fa0..a009182 100644
--- a/internal/config.go
+++ b/internal/config.go
@@ -238,7 +238,7 @@ func (hc *hieraCfg) Resolve(ic hieraapi.Invocation) (cfg hieraapi.ResolvedConfig
 	v := ic.WithLookup(k, func() px.Value {
 		return ms.Lookup(r.Hierarchy(), ic, func(prv interface{}) px.Value {
 			pr := prv.(hieraapi.DataProvider)
-			return pr.UncheckedLookup(k, ic, ms)
+			return pr.Lookup(k, ic, ms)
 		})
 	})
 
diff --git a/internal/datadigprovider.go b/internal/datadigprovider.go
index 67b4df4..639f485 100644
--- a/internal/datadigprovider.go
+++ b/internal/datadigprovider.go
@@ -14,7 +14,7 @@ type DataDigProvider struct {
 	hashes         *sync.Map
 }
 
-func (dh *DataDigProvider) UncheckedLookup(key hieraapi.Key, invocation hieraapi.Invocation, merge hieraapi.MergeStrategy) px.Value {
+func (dh *DataDigProvider) Lookup(key hieraapi.Key, invocation hieraapi.Invocation, merge hieraapi.MergeStrategy) px.Value {
 	return invocation.WithDataProvider(dh, func() px.Value {
 		locations := dh.hierarchyEntry.Locations()
 		switch len(locations) {
diff --git a/internal/datahashprovider.go b/internal/datahashprovider.go
index 0401e65..937540b 100644
--- a/internal/datahashprovider.go
+++ b/internal/datahashprovider.go
@@ -18,7 +18,7 @@ type DataHashProvider struct {
 	hashesLock     sync.RWMutex
 }
 
-func (dh *DataHashProvider) UncheckedLookup(key hieraapi.Key, invocation hieraapi.Invocation, merge hieraapi.MergeStrategy) px.Value {
+func (dh *DataHashProvider) Lookup(key hieraapi.Key, invocation hieraapi.Invocation, merge hieraapi.MergeStrategy) px.Value {
 	return invocation.WithDataProvider(dh, func() px.Value {
 		locations := dh.hierarchyEntry.Locations()
 		switch len(locations) {
diff --git a/internal/init.go b/internal/init.go
index 3948845..d873b45 100644
--- a/internal/init.go
+++ b/internal/init.go
@@ -83,7 +83,7 @@ func Lookup2(
 		if ov, ok := override.Get4(name); ok {
 			return ov
 		}
-		v := ic.(*invocation).lookupViaCache(newKey(name), options)
+		v := ic.(*invocation).lookup(newKey(name), options)
 		if v != nil {
 			return v
 		}
diff --git a/internal/invocation.go b/internal/invocation.go
index afe3a47..4e2ead3 100644
--- a/internal/invocation.go
+++ b/internal/invocation.go
@@ -151,39 +151,46 @@ func (ic *invocation) sharedCache() *sync.Map {
 	panic(px.Error(hieraapi.NotInitialized, issue.NoArgs))
 }
 
-func (ic *invocation) Config() (conf hieraapi.ResolvedConfig) {
+func (ic *invocation) Config() hieraapi.ResolvedConfig {
 	sc := ic.sharedCache()
 	cp := hieraConfigsPrefix + ic.configPath
 	if val, ok := sc.Load(cp); ok {
-		conf = val.(hieraapi.ResolvedConfig)
-		return
+		return val.(hieraapi.Config).Resolve(ic)
 	}
 
 	lc := hieraLockPrefix + ic.configPath
+
 	myLock := sync.RWMutex{}
 	myLock.Lock()
-	defer myLock.Unlock()
 
+	var conf hieraapi.Config
 	if lv, loaded := sc.LoadOrStore(lc, &myLock); loaded {
-		// Only the one storing thread should proceed and create the configuration. This thread
-		// awaits the completion of that creation by waiting for the loaded mutex.
-		lock := lv.(*sync.RWMutex)
-		lock.RLock()
-		val, _ := sc.Load(cp)
-		lock.RUnlock()
-		conf = val.(hieraapi.ResolvedConfig)
+		// myLock was not stored so unlock it
+		myLock.Unlock()
+
+		if lock, ok := lv.(*sync.RWMutex); ok {
+			// The loaded value is a lock. Wait for new config to be stored in place of
+			// this lock
+			lock.RLock()
+			val, _ := sc.Load(cp)
+			conf = val.(hieraapi.Config)
+			lock.RUnlock()
+		} else {
+			conf = lv.(hieraapi.Config)
+		}
 	} else {
-		conf = NewConfig(ic, ic.configPath).Resolve(ic)
+		conf = NewConfig(ic, ic.configPath)
 		sc.Store(cp, conf)
+		myLock.Unlock()
 	}
-	return
+	return conf.Resolve(ic)
 }
 
 func (ic *invocation) ExplainMode() bool {
 	return ic.explainer != nil
 }
 
-func (ic *invocation) lookupViaCache(key hieraapi.Key, options map[string]px.Value) px.Value {
+func (ic *invocation) lookup(key hieraapi.Key, options map[string]px.Value) px.Value {
 	rootKey := key.Root()
 	if rootKey == `lookup_options` {
 		return ic.WithInvalidKey(key, func() px.Value {
@@ -192,11 +199,6 @@ func (ic *invocation) lookupViaCache(key hieraapi.Key, options map[string]px.Val
 		})
 	}
 
-	sc := ic.sharedCache()
-	if val, ok := sc.Load(rootKey); ok {
-		return key.Dig(ic.ForData(), val.(px.Value))
-	}
-
 	globalOptions := globalOptions(ic)
 	if len(options) == 0 {
 		options = globalOptions
@@ -214,7 +216,6 @@ func (ic *invocation) lookupViaCache(key hieraapi.Key, options map[string]px.Val
 	if v != nil {
 		dc := ic.ForData()
 		v = Interpolate(dc, v, true)
-		sc.Store(rootKey, v)
 		v = key.Dig(dc, v)
 	}
 	return v
diff --git a/internal/lookupkeyprovider.go b/internal/lookupkeyprovider.go
index 63e5366..4b7b72d 100644
--- a/internal/lookupkeyprovider.go
+++ b/internal/lookupkeyprovider.go
@@ -17,7 +17,7 @@ type LookupKeyProvider struct {
 	hashes         *sync.Map
 }
 
-func (dh *LookupKeyProvider) UncheckedLookup(key hieraapi.Key, invocation hieraapi.Invocation, merge hieraapi.MergeStrategy) px.Value {
+func (dh *LookupKeyProvider) Lookup(key hieraapi.Key, invocation hieraapi.Invocation, merge hieraapi.MergeStrategy) px.Value {
 	return invocation.WithDataProvider(dh, func() px.Value {
 		locations := dh.hierarchyEntry.Locations()
 		switch len(locations) {
diff --git a/provider/configlookupkey.go b/provider/configlookupkey.go
index 407cd49..be34a6e 100644
--- a/provider/configlookupkey.go
+++ b/provider/configlookupkey.go
@@ -81,7 +81,7 @@ func ConfigLookupKey(pc hieraapi.ProviderContext, key string, options map[string
 			ms := hieraapi.GetMergeStrategy(hieraapi.MergeStrategyName(merge.String()), mergeOpts)
 			v = ms.Lookup(cfg.Hierarchy(), ic, func(prv interface{}) px.Value {
 				pr := prv.(hieraapi.DataProvider)
-				return pr.UncheckedLookup(k, ic, ms)
+				return pr.Lookup(k, ic, ms)
 			})
 		}
 


Test patch:
diff --git a/hieraapi/providercontext_test.go b/hieraapi/providercontext_test.go
index bb84ebb..6ec6894 100644
--- a/hieraapi/providercontext_test.go
+++ b/hieraapi/providercontext_test.go
@@ -18,7 +18,7 @@ func ExampleProviderContext_CachedValue() {
 			return v
 		}
 		fmt.Printf("Creating and caching value for %s\n", key)
-		v := ic.Interpolate(types.WrapString(fmt.Sprintf("generated value for %%{%s}", key)))
+		v := ic.Interpolate(types.WrapString(fmt.Sprintf("value for %%{%s}", key)))
 		ic.Cache(key, v)
 		return v
 	}
@@ -36,9 +36,11 @@ func ExampleProviderContext_CachedValue() {
 	})
 	// Output:
 	// Creating and caching value for a
-	// generated value for scope 'a'
+	// value for scope 'a'
 	// Creating and caching value for b
-	// generated value for scope 'b'
-	// generated value for scope 'a'
-	// generated value for scope 'b'
+	// value for scope 'b'
+	// Returning cached value for a
+	// value for scope 'a'
+	// Returning cached value for b
+	// value for scope 'b'
 }