From 3977d6b7bd9fae800b869569f4b8059bc9c2975d Mon Sep 17 00:00:00 2001 From: Aditya Manthramurthy Date: Mon, 24 Oct 2016 19:52:24 -0700 Subject: [PATCH] Lock bucket while modifying its metadata (Fixes #2979) (#3019) - When modifying notification configuration - When modifying listener configuration - When modifying policy configuration With this change we also stop early checking if the bucket exists, since that uses a Read-lock and causes a deadlock due to the outer Write-lock. --- cmd/bucket-notification-handlers.go | 21 +++++++++++++++++++++ cmd/bucket-policy-handlers.go | 12 ++++++++++-- cmd/bucket-policy.go | 10 ---------- cmd/event-notifier.go | 12 ------------ cmd/event-notifier_test.go | 9 --------- 5 files changed, 31 insertions(+), 33 deletions(-) diff --git a/cmd/bucket-notification-handlers.go b/cmd/bucket-notification-handlers.go index 4fdb29bbb..c5c75bcdb 100644 --- a/cmd/bucket-notification-handlers.go +++ b/cmd/bucket-notification-handlers.go @@ -164,6 +164,13 @@ func PutBucketNotificationConfig(bucket string, ncfg *notificationConfig, objAPI return errInvalidArgument } + // Acquire a write lock on bucket before modifying its + // configuration. + opsID := getOpsID() + nsMutex.Lock(bucket, "", opsID) + // Release lock after notifying peers + defer nsMutex.Unlock(bucket, "", opsID) + // persist config to disk err := persistNotificationConfig(bucket, ncfg, objAPI) if err != nil { @@ -365,6 +372,13 @@ func AddBucketListenerConfig(bucket string, lcfg *listenerConfig, objAPI ObjectL // add new lid to listeners and persist to object layer. listenerCfgs = append(listenerCfgs, *lcfg) + // Acquire a write lock on bucket before modifying its + // configuration. + opsID := getOpsID() + nsMutex.Lock(bucket, "", opsID) + // Release lock after notifying peers + defer nsMutex.Unlock(bucket, "", opsID) + // update persistent config err := persistListenerConfig(bucket, listenerCfgs, objAPI) if err != nil { @@ -398,6 +412,13 @@ func RemoveBucketListenerConfig(bucket string, lcfg *listenerConfig, objAPI Obje return } + // Acquire a write lock on bucket before modifying its + // configuration. + opsID := getOpsID() + nsMutex.Lock(bucket, "", opsID) + // Release lock after notifying peers + defer nsMutex.Unlock(bucket, "", opsID) + // update persistent config err := persistListenerConfig(bucket, updatedLcfgs, objAPI) if err != nil { diff --git a/cmd/bucket-policy-handlers.go b/cmd/bucket-policy-handlers.go index b37678f5d..10bb51896 100644 --- a/cmd/bucket-policy-handlers.go +++ b/cmd/bucket-policy-handlers.go @@ -200,12 +200,20 @@ func (api objectAPIHandlers) PutBucketPolicyHandler(w http.ResponseWriter, r *ht // persists it to storage, and notify nodes in the cluster about the // change. In-memory state is updated in response to the notification. func persistAndNotifyBucketPolicyChange(bucket string, pCh policyChange, objAPI ObjectLayer) error { - // FIXME: Race exists between the bucket existence check and - // then updating the bucket policy. + // Verify if bucket actually exists. FIXME: Ideally this check + // should not be used but is kept here to error out for + // invalid and non-existent buckets. if err := isBucketExist(bucket, objAPI); err != nil { return err } + // Acquire a write lock on bucket before modifying its + // configuration. + opsID := getOpsID() + nsMutex.Lock(bucket, "", opsID) + // Release lock after notifying peers + defer nsMutex.Unlock(bucket, "", opsID) + if pCh.IsRemove { if err := removeBucketPolicy(bucket, objAPI); err != nil { return err diff --git a/cmd/bucket-policy.go b/cmd/bucket-policy.go index c22dddac5..3b3155e19 100644 --- a/cmd/bucket-policy.go +++ b/cmd/bucket-policy.go @@ -193,11 +193,6 @@ func readBucketPolicy(bucket string, objAPI ObjectLayer) (*bucketPolicy, error) // removeBucketPolicy - removes any previously written bucket policy. Returns BucketPolicyNotFound // if no policies are found. func removeBucketPolicy(bucket string, objAPI ObjectLayer) error { - // Verify if bucket actually exists - if err := isBucketExist(bucket, objAPI); err != nil { - return err - } - policyPath := pathJoin(bucketConfigPrefix, bucket, policyJSON) if err := objAPI.DeleteObject(minioMetaBucket, policyPath); err != nil { errorIf(err, "Unable to remove bucket-policy on bucket %s.", bucket) @@ -213,11 +208,6 @@ func removeBucketPolicy(bucket string, objAPI ObjectLayer) error { // writeBucketPolicy - save a bucket policy that is assumed to be // validated. func writeBucketPolicy(bucket string, objAPI ObjectLayer, bpy *bucketPolicy) error { - // Verify if bucket actually exists - if err := isBucketExist(bucket, objAPI); err != nil { - return err - } - buf, err := json.Marshal(bpy) if err != nil { errorIf(err, "Unable to marshal bucket policy '%v' to JSON", *bpy) diff --git a/cmd/event-notifier.go b/cmd/event-notifier.go index 1eb59cbea..ed86acd8a 100644 --- a/cmd/event-notifier.go +++ b/cmd/event-notifier.go @@ -399,12 +399,6 @@ func persistNotificationConfig(bucket string, ncfg *notificationConfig, obj Obje return err } - // verify bucket exists - // FIXME: There is a race between this check and PutObject - if err = isBucketExist(bucket, obj); err != nil { - return err - } - // build path ncPath := path.Join(bucketConfigPrefix, bucket, bucketNotificationConfig) // write object to path @@ -425,12 +419,6 @@ func persistListenerConfig(bucket string, lcfg []listenerConfig, obj ObjectLayer return err } - // verify bucket exists - // FIXME: There is a race between this check and PutObject - if err = isBucketExist(bucket, obj); err != nil { - return err - } - // build path lcPath := path.Join(bucketConfigPrefix, bucket, bucketListenerConfig) // write object to path diff --git a/cmd/event-notifier_test.go b/cmd/event-notifier_test.go index e3b5c4e97..be2beab45 100644 --- a/cmd/event-notifier_test.go +++ b/cmd/event-notifier_test.go @@ -277,15 +277,6 @@ func TestInitEventNotifier(t *testing.T) { }, } - // write without an existing bucket and check - if err := persistNotificationConfig(bucketName, ¬ificationConfig{}, obj); err == nil { - t.Fatalf("Did not get an error though bucket does not exist!") - } - // no bucket write check for listener - if err := persistListenerConfig(bucketName, []listenerConfig{}, obj); err == nil { - t.Fatalf("Did not get an error though bucket does not exist!") - } - // create bucket if err := obj.MakeBucket(bucketName); err != nil { t.Fatal("Unexpected error:", err)