usagepool: avoid lock inversion after constructor failure (#7968)

usagepool.go

@@ -94,18 +94,18 @@ func (up *UsagePool) LoadOrNew(key any, construct Constructor) (value any, loade
 		value, err = construct()
 		if err == nil {
 			upv.value = value
+			upv.Unlock()
 		} else {
 			upv.err = err
+			upv.Unlock()
 			up.Lock()
-			// this *should* be safe, I think, because we have a
-			// write lock on upv, but we might also need to ensure
-			// that upv.err is nil before doing this, since we
-			// released the write lock on up during construct...
-			// but then again it's also after midnight...
-			delete(up.pool, key)
+			// Another constructor may have replaced a failed entry while
+			// this goroutine waited for the pool lock.
+			if up.pool[key] == upv {
+				delete(up.pool, key)
+			}
 			up.Unlock()
 		}
-		upv.Unlock()
 	}
 	return value, loaded, err
 }

usagepool_test.go

@@ -0,0 +1,63 @@
+// Copyright 2015 Matthew Holt and The Caddy Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package caddy
+
+import (
+	"errors"
+	"testing"
+	"time"
+)
+
+func TestUsagePoolFailedConstructorDoesNotInvertLocks(t *testing.T) {
+	pool := NewUsagePool()
+	constructorStarted := make(chan struct{})
+	finishConstructor := make(chan struct{})
+	loadResult := make(chan error, 1)
+	constructErr := errors.New("construction failed")
+
+	go func() {
+		_, _, err := pool.LoadOrNew("key", func() (Destructor, error) {
+			close(constructorStarted)
+			<-finishConstructor
+			return nil, constructErr
+		})
+		loadResult <- err
+	}()
+
+	<-constructorStarted
+	pool.RLock()
+	entry := pool.pool["key"]
+	close(finishConstructor)
+
+	entryReadable := make(chan struct{})
+	go func() {
+		entry.RLock()
+		entry.RUnlock()
+		close(entryReadable)
+	}()
+
+	select {
+	case <-entryReadable:
+		pool.RUnlock()
+	case <-time.After(time.Second):
+		pool.RUnlock()
+		<-loadResult
+		t.Fatal("failed constructor held the entry lock while waiting for the pool lock")
+	}
+
+	if err := <-loadResult; !errors.Is(err, constructErr) {
+		t.Fatalf("LoadOrNew() error = %v, want %v", err, constructErr)
+	}
+}