From 8aefdae06f570ecbc0bd831d7fc9fb6b1511f6b9 Mon Sep 17 00:00:00 2001 From: Shiwei Zhang Date: Tue, 19 Mar 2024 18:27:17 +0800 Subject: [PATCH 1/4] fix: retry setCredsStore on next call Signed-off-by: Shiwei Zhang --- registry/remote/credentials/store.go | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/registry/remote/credentials/store.go b/registry/remote/credentials/store.go index ae8ce5be..b71d9acb 100644 --- a/registry/remote/credentials/store.go +++ b/registry/remote/credentials/store.go @@ -53,7 +53,7 @@ type DynamicStore struct { config *config.Config options StoreOptions detectedCredsStore string - setCredsStoreOnce sync.Once + setCredsStoreOnce func() error } // StoreOptions provides options for NewStore. @@ -107,6 +107,18 @@ func NewStore(configPath string, opts StoreOptions) (*DynamicStore, error) { // no authentication configured, detect the default credentials store ds.detectedCredsStore = getDefaultHelperSuffix() } + var setCredsStore func() error + setCredsStore = func() error { + if ds.detectedCredsStore != "" { + if err := ds.config.SetCredentialsStore(ds.detectedCredsStore); err != nil { + // retry on next call + ds.setCredsStoreOnce = sync.OnceValue(setCredsStore) + return fmt.Errorf("failed to set credsStore: %w", err) + } + } + return nil + } + ds.setCredsStoreOnce = sync.OnceValue(setCredsStore) return ds, nil } @@ -136,19 +148,12 @@ func (ds *DynamicStore) Get(ctx context.Context, serverAddress string) (auth.Cre // Put saves credentials into the store for the given server address. // Put returns ErrPlaintextPutDisabled if native store is not available and // [StoreOptions].AllowPlaintextPut is set to false. -func (ds *DynamicStore) Put(ctx context.Context, serverAddress string, cred auth.Credential) (returnErr error) { +func (ds *DynamicStore) Put(ctx context.Context, serverAddress string, cred auth.Credential) error { if err := ds.getStore(serverAddress).Put(ctx, serverAddress, cred); err != nil { return err } // save the detected creds store back to the config file on first put - ds.setCredsStoreOnce.Do(func() { - if ds.detectedCredsStore != "" { - if err := ds.config.SetCredentialsStore(ds.detectedCredsStore); err != nil { - returnErr = fmt.Errorf("failed to set credsStore: %w", err) - } - } - }) - return returnErr + return ds.setCredsStoreOnce() } // Delete removes credentials from the store for the given server address. From 2890d86cefdbcdfc5cbe0467a7adccde78c49cdd Mon Sep 17 00:00:00 2001 From: Shiwei Zhang Date: Tue, 19 Mar 2024 21:27:32 +0800 Subject: [PATCH 2/4] fix: correct race conditions Signed-off-by: Shiwei Zhang --- internal/syncutil/oncefunc.go | 33 ++++++++++++++++++++++++++++ registry/remote/credentials/store.go | 10 +++------ 2 files changed, 36 insertions(+), 7 deletions(-) create mode 100644 internal/syncutil/oncefunc.go diff --git a/internal/syncutil/oncefunc.go b/internal/syncutil/oncefunc.go new file mode 100644 index 00000000..004b5138 --- /dev/null +++ b/internal/syncutil/oncefunc.go @@ -0,0 +1,33 @@ +package syncutil + +import ( + "sync" + "sync/atomic" +) + +// OnceRetryOnError returns a function that invokes f only once if f returns +// nil. Otherwise, it retries on the next call. +func OnceRetryOnError(f func() error) func() error { + var done atomic.Bool + var lock sync.Mutex + + return func() error { + // fast path + if done.Load() { + return nil + } + + // slow path + lock.Lock() + defer lock.Unlock() + + if done.Load() { + return nil + } + if err := f(); err != nil { + return err + } + done.Store(true) + return nil + } +} diff --git a/registry/remote/credentials/store.go b/registry/remote/credentials/store.go index b71d9acb..7c4881d3 100644 --- a/registry/remote/credentials/store.go +++ b/registry/remote/credentials/store.go @@ -25,8 +25,8 @@ import ( "fmt" "os" "path/filepath" - "sync" + "oras.land/oras-go/v2/internal/syncutil" "oras.land/oras-go/v2/registry/remote/auth" "oras.land/oras-go/v2/registry/remote/credentials/internal/config" ) @@ -107,18 +107,14 @@ func NewStore(configPath string, opts StoreOptions) (*DynamicStore, error) { // no authentication configured, detect the default credentials store ds.detectedCredsStore = getDefaultHelperSuffix() } - var setCredsStore func() error - setCredsStore = func() error { + ds.setCredsStoreOnce = syncutil.OnceRetryOnError(func() error { if ds.detectedCredsStore != "" { if err := ds.config.SetCredentialsStore(ds.detectedCredsStore); err != nil { - // retry on next call - ds.setCredsStoreOnce = sync.OnceValue(setCredsStore) return fmt.Errorf("failed to set credsStore: %w", err) } } return nil - } - ds.setCredsStoreOnce = sync.OnceValue(setCredsStore) + }) return ds, nil } From dab300ff0d11980d68b4b562b410bf29e3ea234e Mon Sep 17 00:00:00 2001 From: Shiwei Zhang Date: Tue, 19 Mar 2024 22:44:04 +0800 Subject: [PATCH 3/4] refactor: simplify Once code Signed-off-by: Shiwei Zhang --- internal/syncutil/once.go | 36 ++++++++++++++++++++++++++-- internal/syncutil/oncefunc.go | 33 ------------------------- registry/remote/credentials/store.go | 19 +++++++-------- 3 files changed, 43 insertions(+), 45 deletions(-) delete mode 100644 internal/syncutil/oncefunc.go diff --git a/internal/syncutil/once.go b/internal/syncutil/once.go index 1d557198..e4497053 100644 --- a/internal/syncutil/once.go +++ b/internal/syncutil/once.go @@ -15,10 +15,14 @@ limitations under the License. package syncutil -import "context" +import ( + "context" + "sync" + "sync/atomic" +) // Once is an object that will perform exactly one action. -// Unlike sync.Once, this Once allowes the action to have return values. +// Unlike sync.Once, this Once allows the action to have return values. type Once struct { result interface{} err error @@ -68,3 +72,31 @@ func (o *Once) Do(ctx context.Context, f func() (interface{}, error)) (bool, int } } } + +// OnceOrRetry is an object that will perform exactly one success action. +type OnceOrRetry struct { + done atomic.Bool + lock sync.Mutex +} + +// OnceOrRetry calls the function f if and only if Do is being called for the +// first time for this instance of Once or all previous calls to Do are failed. +func (o *OnceOrRetry) Do(f func() error) error { + // fast path + if o.done.Load() { + return nil + } + + // slow path + o.lock.Lock() + defer o.lock.Unlock() + + if o.done.Load() { + return nil + } + if err := f(); err != nil { + return err + } + o.done.Store(true) + return nil +} diff --git a/internal/syncutil/oncefunc.go b/internal/syncutil/oncefunc.go deleted file mode 100644 index 004b5138..00000000 --- a/internal/syncutil/oncefunc.go +++ /dev/null @@ -1,33 +0,0 @@ -package syncutil - -import ( - "sync" - "sync/atomic" -) - -// OnceRetryOnError returns a function that invokes f only once if f returns -// nil. Otherwise, it retries on the next call. -func OnceRetryOnError(f func() error) func() error { - var done atomic.Bool - var lock sync.Mutex - - return func() error { - // fast path - if done.Load() { - return nil - } - - // slow path - lock.Lock() - defer lock.Unlock() - - if done.Load() { - return nil - } - if err := f(); err != nil { - return err - } - done.Store(true) - return nil - } -} diff --git a/registry/remote/credentials/store.go b/registry/remote/credentials/store.go index 7c4881d3..e26a98ae 100644 --- a/registry/remote/credentials/store.go +++ b/registry/remote/credentials/store.go @@ -53,7 +53,7 @@ type DynamicStore struct { config *config.Config options StoreOptions detectedCredsStore string - setCredsStoreOnce func() error + setCredsStoreOnce syncutil.OnceOrRetry } // StoreOptions provides options for NewStore. @@ -107,14 +107,6 @@ func NewStore(configPath string, opts StoreOptions) (*DynamicStore, error) { // no authentication configured, detect the default credentials store ds.detectedCredsStore = getDefaultHelperSuffix() } - ds.setCredsStoreOnce = syncutil.OnceRetryOnError(func() error { - if ds.detectedCredsStore != "" { - if err := ds.config.SetCredentialsStore(ds.detectedCredsStore); err != nil { - return fmt.Errorf("failed to set credsStore: %w", err) - } - } - return nil - }) return ds, nil } @@ -149,7 +141,14 @@ func (ds *DynamicStore) Put(ctx context.Context, serverAddress string, cred auth return err } // save the detected creds store back to the config file on first put - return ds.setCredsStoreOnce() + return ds.setCredsStoreOnce.Do(func() error { + if ds.detectedCredsStore != "" { + if err := ds.config.SetCredentialsStore(ds.detectedCredsStore); err != nil { + return fmt.Errorf("failed to set credsStore: %w", err) + } + } + return nil + }) } // Delete removes credentials from the store for the given server address. From 04b74081ba49ba1e9bd3a12f801e7423ed5b8379 Mon Sep 17 00:00:00 2001 From: Shiwei Zhang Date: Tue, 19 Mar 2024 23:17:20 +0800 Subject: [PATCH 4/4] test: add test for OnceOrRetry Signed-off-by: Shiwei Zhang --- internal/syncutil/once_test.go | 95 ++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/internal/syncutil/once_test.go b/internal/syncutil/once_test.go index 2bcba2f1..461c1e68 100644 --- a/internal/syncutil/once_test.go +++ b/internal/syncutil/once_test.go @@ -22,6 +22,7 @@ import ( "reflect" "strconv" "sync" + "sync/atomic" "testing" "time" ) @@ -191,3 +192,97 @@ func TestOnce_Do_Cancel_Panic(t *testing.T) { t.Fatalf("Once.Do() result = %v, want %v", result, wantResult) } } + +func TestOnceOrRetry_Do(t *testing.T) { + var once OnceOrRetry + var count atomic.Int32 + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + err := once.Do(func() error { + count.Add(1) + return nil + }) + if err != nil { + t.Errorf("OnceOrRetry.Do() error = %v, wantErr %v", err, nil) + } + }() + } + wg.Wait() + + if got := count.Load(); got != 1 { + t.Fatal("OnceOrRetry.Do() called more than once") + } +} + +func TestOnceOrRetry_Do_Fail(t *testing.T) { + var once OnceOrRetry + var wg sync.WaitGroup + + // test failure + for i := 0; i < 100; i++ { + wg.Add(1) + go func(wantErr error) { + defer wg.Done() + err := once.Do(func() error { + return wantErr + }) + if err != wantErr { + t.Errorf("OnceOrRetry.Do() error = %v, wantErr %v", err, wantErr) + } + }(errors.New(strconv.Itoa(i))) + } + wg.Wait() + + // retry after failure + err := once.Do(func() error { + return nil + }) + if err != nil { + t.Fatalf("OnceOrRetry.Do() error = %v, wantErr %v", err, nil) + } + + // no retry after success + err = once.Do(func() error { + t.Fatal("OnceOrRetry.Do() called twice") + return nil + }) + if err != nil { + t.Fatalf("OnceOrRetry.Do() error = %v, wantErr %v", err, nil) + } +} + +func TestOnceOrRetry_Do_Panic(t *testing.T) { + var once OnceOrRetry + + // test panic + func() { + defer func() { + if r := recover(); r == nil { + t.Fatal("OnceOrRetry.Do() did not panic") + } + }() + _ = once.Do(func() error { + panic("failed") + }) + }() + + // retry after panic + err := once.Do(func() error { + return nil + }) + if err != nil { + t.Fatalf("OnceOrRetry.Do() error = %v, wantErr %v", err, nil) + } + + // no retry after success + err = once.Do(func() error { + t.Fatal("OnceOrRetry.Do() called twice") + return nil + }) + if err != nil { + t.Fatalf("OnceOrRetry.Do() error = %v, wantErr %v", err, nil) + } +}