diff --git a/caddytls/client.go b/caddytls/client.go index 3a9adae6..26ef6a3c 100644 --- a/caddytls/client.go +++ b/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) } }() diff --git a/caddytls/filestorage.go b/caddytls/filestorage.go index 7e8d730e..67084ef4 100644 --- a/caddytls/filestorage.go +++ b/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), - nameLocks: make(map[string]*sync.WaitGroup), + Path: filepath.Join(storageBasePath, caURL.Host), }, 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 - nameLocks map[string]*sync.WaitGroup - nameLocksMu sync.Mutex + Path string } // 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 diff --git a/caddytls/storage.go b/caddytls/storage.go index 666d3f06..8587dd02 100644 --- a/caddytls/storage.go +++ b/caddytls/storage.go @@ -39,24 +39,9 @@ type UserData struct { Key []byte } -// 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) - - // 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. - // +// Locker provides support for mutual exclusion +type Locker interface { + // TryLock will return immediatedly with or without acquiring the lock. // 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 diff --git a/caddytls/sync_locker.go b/caddytls/sync_locker.go new file mode 100644 index 00000000..693f3b87 --- /dev/null +++ b/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 +} diff --git a/caddytls/tls_test.go b/caddytls/tls_test.go index 7eb5c3b2..2b592cf5 100644 --- a/caddytls/tls_test.go +++ b/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 { diff --git a/caddytls/user_test.go b/caddytls/user_test.go index 23a45edc..f82480fb 100644 --- a/caddytls/user_test.go +++ b/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)