From 82040fdb58c8b70f0da7c0c3a9b428205f52a562 Mon Sep 17 00:00:00 2001
From: Matt Holt <mholt@users.noreply.github.com>
Date: Wed, 27 May 2020 15:05:53 -0600
Subject: [PATCH] Lock now takes a context and should honor cancellation (#66)

* Lock now takes a context and should honor cancellation

This allows callers to give up if they can't obtain a lock in a certain
timeframe and for resources to be cleaned up, avoiding potential
resource leaks.

Breaking change for any Storage implementations, sorry about that. (It's
why we're not 1.0 yet.) I'll reach out to known implementations; it's a
simple change.

* Rename obtainLock to acquireLock to be less ambiguous

In our package, "obtain" has a more common meaning related to certs
---
 config.go      |  4 ++--
 filestorage.go | 12 +++++++++---
 storage.go     | 12 ++++++++----
 3 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/config.go b/config.go
index b412b8f4..01bd1eb9 100644
--- a/config.go
+++ b/config.go
@@ -388,7 +388,7 @@ func (cfg *Config) obtainWithIssuer(ctx context.Context, issuer Issuer, name str
 
 	// ensure idempotency of the obtain operation for this name
 	lockKey := cfg.lockKey("cert_acme", name)
-	err := obtainLock(cfg.Storage, lockKey)
+	err := acquireLock(ctx, cfg.Storage, lockKey)
 	if err != nil {
 		return err
 	}
@@ -474,7 +474,7 @@ func (cfg *Config) renewWithIssuer(ctx context.Context, issuer Issuer, name stri
 
 	// ensure idempotency of the renew operation for this name
 	lockKey := cfg.lockKey("cert_acme", name)
-	err := obtainLock(cfg.Storage, lockKey)
+	err := acquireLock(ctx, cfg.Storage, lockKey)
 	if err != nil {
 		return err
 	}
diff --git a/filestorage.go b/filestorage.go
index ae46dff5..f3603d07 100644
--- a/filestorage.go
+++ b/filestorage.go
@@ -15,6 +15,7 @@
 package certmagic
 
 import (
+	"context"
 	"encoding/json"
 	"fmt"
 	"io"
@@ -124,7 +125,7 @@ func (fs *FileStorage) Filename(key string) string {
 
 // Lock obtains a lock named by the given key. It blocks
 // until the lock can be obtained or an error is returned.
-func (fs *FileStorage) Lock(key string) error {
+func (fs *FileStorage) Lock(ctx context.Context, key string) error {
 	filename := fs.lockFilename(key)
 
 	for {
@@ -168,8 +169,13 @@ func (fs *FileStorage) Lock(key string) error {
 
 		default:
 			// lockfile exists and is not stale;
-			// just wait a moment and try again
-			time.Sleep(fileLockPollInterval)
+			// just wait a moment and try again,
+			// or return if context cancelled
+			select {
+			case <-time.After(fileLockPollInterval):
+			case <-ctx.Done():
+				return ctx.Err()
+			}
 		}
 	}
 }
diff --git a/storage.go b/storage.go
index 4b431bc6..f5045a97 100644
--- a/storage.go
+++ b/storage.go
@@ -15,6 +15,7 @@
 package certmagic
 
 import (
+	"context"
 	"log"
 	"path"
 	"regexp"
@@ -86,8 +87,11 @@ type Locker interface {
 	// To prevent deadlocks, all implementations (where this concern
 	// is relevant) should put a reasonable expiration on the lock in
 	// case Unlock is unable to be called due to some sort of network
-	// failure or system crash.
-	Lock(key string) error
+	// failure or system crash. Additionally, implementations should
+	// honor context cancellation as much as possible (in case the
+	// caller wishes to give up and free resources before the lock
+	// can be obtained).
+	Lock(ctx context.Context, key string) error
 
 	// Unlock releases the lock for key. This method must ONLY be
 	// called after a successful call to Lock, and only after the
@@ -223,8 +227,8 @@ func CleanUpOwnLocks() {
 	}
 }
 
-func obtainLock(storage Storage, lockKey string) error {
-	err := storage.Lock(lockKey)
+func acquireLock(ctx context.Context, storage Storage, lockKey string) error {
+	err := storage.Lock(ctx, lockKey)
 	if err == nil {
 		locksMu.Lock()
 		locks[lockKey] = storage