caddytls: Extract locker into an interface (#1942)

caddytls/client.go

@@ -39,6 +39,7 @@ type ACMEClient struct {
 	AllowPrompts bool
 	config       *Config
 	acmeClient   *acme.Client
+	locker       Locker
 }
 
 // newACMEClient creates a new ACMEClient given an email and whether
@@ -120,6 +121,10 @@ var newACMEClient = func(config *Config, allowPrompts bool) (*ACMEClient, error)
 		AllowPrompts: allowPrompts,
 		config:       config,
 		acmeClient:   client,
+		locker: &syncLock{
+			nameLocks:   make(map[string]*sync.WaitGroup),
+			nameLocksMu: sync.Mutex{},
+		},
 	}
 
 	if config.DNSProvider == "" {
@@ -210,7 +215,7 @@ func (c *ACMEClient) Obtain(name string) error {
 		return err
 	}
 
-	waiter, err := storage.TryLock(name)
+	waiter, err := c.locker.TryLock(name)
 	if err != nil {
 		return err
 	}
@@ -220,7 +225,7 @@ func (c *ACMEClient) Obtain(name string) error {
 		return nil // we assume the process with the lock succeeded, rather than hammering this execution path again
 	}
 	defer func() {
-		if err := storage.Unlock(name); err != nil {
+		if err := c.locker.Unlock(name); err != nil {
 			log.Printf("[ERROR] Unable to unlock obtain call for %s: %v", name, err)
 		}
 	}()
@@ -286,7 +291,7 @@ func (c *ACMEClient) Renew(name string) error {
 		return err
 	}
 
-	waiter, err := storage.TryLock(name)
+	waiter, err := c.locker.TryLock(name)
 	if err != nil {
 		return err
 	}
@@ -296,7 +301,7 @@ func (c *ACMEClient) Renew(name string) error {
 		return nil // we assume the process with the lock succeeded, rather than hammering this execution path again
 	}
 	defer func() {
-		if err := storage.Unlock(name); err != nil {
+		if err := c.locker.Unlock(name); err != nil {
 			log.Printf("[ERROR] Unable to unlock renew call for %s: %v", name, err)
 		}
 	}()

caddytls/filestorage.go

@@ -22,7 +22,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"sync"
 
 	"github.com/mholt/caddy"
 )
@@ -40,8 +39,7 @@ var storageBasePath = filepath.Join(caddy.AssetsPath(), "acme")
 // instance is guaranteed to be non-nil if there is no error.
 func NewFileStorage(caURL *url.URL) (Storage, error) {
 	return &FileStorage{
-		Path:      filepath.Join(storageBasePath, caURL.Host),
+		Path: filepath.Join(storageBasePath, caURL.Host),
-		nameLocks: make(map[string]*sync.WaitGroup),
 	}, nil
 }
 
@@ -49,9 +47,7 @@ func NewFileStorage(caURL *url.URL) (Storage, error) {
 // directory. It is used to get file paths in a consistent,
 // cross-platform way or persisting ACME assets on the file system.
 type FileStorage struct {
-	Path        string
+	Path string
-	nameLocks   map[string]*sync.WaitGroup
-	nameLocksMu sync.Mutex
 }
 
 // sites gets the directory that stores site certificate and keys.
@@ -254,36 +250,6 @@ func (s *FileStorage) StoreUser(email string, data *UserData) error {
 	return nil
 }
 
-// TryLock attempts to get a lock for name, otherwise it returns
-// a Waiter value to wait until the other process is finished.
-func (s *FileStorage) TryLock(name string) (Waiter, error) {
-	s.nameLocksMu.Lock()
-	defer s.nameLocksMu.Unlock()
-	wg, ok := s.nameLocks[name]
-	if ok {
-		// lock already obtained, let caller wait on it
-		return wg, nil
-	}
-	// caller gets lock
-	wg = new(sync.WaitGroup)
-	wg.Add(1)
-	s.nameLocks[name] = wg
-	return nil, nil
-}
-
-// Unlock unlocks name.
-func (s *FileStorage) Unlock(name string) error {
-	s.nameLocksMu.Lock()
-	defer s.nameLocksMu.Unlock()
-	wg, ok := s.nameLocks[name]
-	if !ok {
-		return fmt.Errorf("FileStorage: no lock to release for %s", name)
-	}
-	wg.Done()
-	delete(s.nameLocks, name)
-	return nil
-}
-
 // MostRecentUserEmail implements Storage.MostRecentUserEmail by finding the
 // most recently written sub directory in the users' directory. It is named
 // after the email address. This corresponds to the most recent call to

caddytls/storage.go

@@ -39,24 +39,9 @@ type UserData struct {
 	Key []byte
 }
 
-// Storage is an interface abstracting all storage used by Caddy's TLS
+// Locker provides support for mutual exclusion
-// subsystem. Implementations of this interface store both site and
+type Locker interface {
-// user data.
+	// TryLock will return immediatedly with or without acquiring the lock.
-type Storage interface {
-	// SiteExists returns true if this site exists in storage.
-	// Site data is considered present when StoreSite has been called
-	// successfully (without DeleteSite having been called, of course).
-	SiteExists(domain string) (bool, error)
-
-	// TryLock is called before Caddy attempts to obtain or renew a
-	// certificate for a certain name and store it. From the perspective
-	// of this method and its companion Unlock, the actions of
-	// obtaining/renewing and then storing the certificate are atomic,
-	// and both should occur within a lock. This prevents multiple
-	// processes -- maybe distributed ones -- from stepping on each
-	// other's space in the same shared storage, and from spamming
-	// certificate providers with multiple, redundant requests.
-	//
 	// If a lock could be obtained, (nil, nil) is returned and you may
 	// continue normally. If not (meaning another process is already
 	// working on that name), a Waiter value will be returned upon
@@ -75,6 +60,16 @@ type Storage interface {
 	// the obtain/renew and store are finished, even if there was
 	// an error (or a timeout).
 	Unlock(name string) error
+}
+
+// Storage is an interface abstracting all storage used by Caddy's TLS
+// subsystem. Implementations of this interface store both site and
+// user data.
+type Storage interface {
+	// SiteExists returns true if this site exists in storage.
+	// Site data is considered present when StoreSite has been called
+	// successfully (without DeleteSite having been called, of course).
+	SiteExists(domain string) (bool, error)
 
 	// LoadSite obtains the site data from storage for the given domain and
 	// returns it. If data for the domain does not exist, an error value

caddytls/sync_locker.go

@@ -0,0 +1,57 @@
+// Copyright 2015 Light Code Labs, LLC
+//
+// 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 caddytls
+
+import (
+	"fmt"
+	"sync"
+)
+
+var _ Locker = &syncLock{}
+
+type syncLock struct {
+	nameLocks   map[string]*sync.WaitGroup
+	nameLocksMu sync.Mutex
+}
+
+// TryLock attempts to get a lock for name, otherwise it returns
+// a Waiter value to wait until the other process is finished.
+func (s *syncLock) TryLock(name string) (Waiter, error) {
+	s.nameLocksMu.Lock()
+	defer s.nameLocksMu.Unlock()
+	wg, ok := s.nameLocks[name]
+	if ok {
+		// lock already obtained, let caller wait on it
+		return wg, nil
+	}
+	// caller gets lock
+	wg = new(sync.WaitGroup)
+	wg.Add(1)
+	s.nameLocks[name] = wg
+	return nil, nil
+}
+
+// Unlock unlocks name.
+func (s *syncLock) Unlock(name string) error {
+	s.nameLocksMu.Lock()
+	defer s.nameLocksMu.Unlock()
+	wg, ok := s.nameLocks[name]
+	if !ok {
+		return fmt.Errorf("FileStorage: no lock to release for %s", name)
+	}
+	wg.Done()
+	delete(s.nameLocks, name)
+	return nil
+}

caddytls/tls_test.go

@@ -16,7 +16,6 @@ package caddytls
 
 import (
 	"os"
-	"sync"
 	"testing"
 
 	"github.com/xenolf/lego/acme"
@@ -94,7 +93,7 @@ func TestQualifiesForManagedTLS(t *testing.T) {
 }
 
 func TestSaveCertResource(t *testing.T) {
-	storage := &FileStorage{Path: "./le_test_save", nameLocks: make(map[string]*sync.WaitGroup)}
+	storage := &FileStorage{Path: "./le_test_save"}
 	defer func() {
 		err := os.RemoveAll(storage.Path)
 		if err != nil {
@@ -140,7 +139,7 @@ func TestSaveCertResource(t *testing.T) {
 }
 
 func TestExistingCertAndKey(t *testing.T) {
-	storage := &FileStorage{Path: "./le_test_existing", nameLocks: make(map[string]*sync.WaitGroup)}
+	storage := &FileStorage{Path: "./le_test_existing"}
 	defer func() {
 		err := os.RemoveAll(storage.Path)
 		if err != nil {

caddytls/user_test.go

@@ -21,7 +21,6 @@ import (
 	"crypto/rand"
 	"io"
 	"strings"
-	"sync"
 	"testing"
 	"time"
 
@@ -196,7 +195,7 @@ func TestGetEmail(t *testing.T) {
 	}
 }
 
-var testStorage = &FileStorage{Path: "./testdata", nameLocks: make(map[string]*sync.WaitGroup)}
+var testStorage = &FileStorage{Path: "./testdata"}
 
 func (s *FileStorage) clean() error {
 	return os.RemoveAll(s.Path)