diff --git a/NEXT_CHANGELOG.md b/NEXT_CHANGELOG.md index 1324d9891..3394f909a 100644 --- a/NEXT_CHANGELOG.md +++ b/NEXT_CHANGELOG.md @@ -8,6 +8,8 @@ ### Bug Fixes +* Fixed `Config` race condition by replacing double-checked locking with `sync.Once` ([#1465](https://github.com/databricks/databricks-sdk-go/pull/1465)). This eliminates race detector warnings and enables proper concurrent usage of the SDK. + ### Documentation ### Internal Changes diff --git a/config/config.go b/config/config.go index 6405cdad4..a7c03174c 100644 --- a/config/config.go +++ b/config/config.go @@ -228,6 +228,12 @@ type Config struct { // This mutex is also used for config resolution. mu sync.Mutex + // resolveAuthOnce ensures authentication happens exactly once. + resolveAuthOnce sync.Once + + // resolveAuthErr stores the error from the authentication attempt. + resolveAuthErr error + // HTTP request interceptor, that assigns Authorization header credentialsProvider credentials.CredentialsProvider @@ -500,16 +506,19 @@ func (c *Config) wrapDebug(err error) error { return fmt.Errorf("%w. %s", err, debug) } -// authenticateIfNeeded lazily authenticates across authorizers or returns error +// authenticateIfNeeded lazily authenticates across authorizers or returns error. +// It uses sync.Once to ensure authentication happens exactly once and is safe +// for concurrent use. func (c *Config) authenticateIfNeeded() error { - if c.credentialsProvider != nil { - return nil - } - c.mu.Lock() - defer c.mu.Unlock() - if c.credentialsProvider != nil { - return nil - } + c.resolveAuthOnce.Do(func() { + c.resolveAuthErr = c.doAuthenticate() + }) + return c.resolveAuthErr +} + +// doAuthenticate performs the actual authentication initialization. +// This method is called exactly once by authenticateIfNeeded via sync.Once. +func (c *Config) doAuthenticate() error { if c.Credentials == nil { c.Credentials = &DefaultCredentials{} } diff --git a/config/config_concurrency_test.go b/config/config_concurrency_test.go new file mode 100644 index 000000000..878711183 --- /dev/null +++ b/config/config_concurrency_test.go @@ -0,0 +1,140 @@ +package config + +import ( + "context" + "net/http" + "sync" + "testing" + + "github.com/databricks/databricks-sdk-go/config/credentials" +) + +// TestAuthenticateConcurrency verifies that Config.Authenticate is safe for +// concurrent use and does not trigger race detector warnings. +func TestAuthenticateConcurrency(t *testing.T) { + cfg := &Config{ + Host: "https://test.cloud.databricks.com", + Token: "fake-token", + } + + // Ensure config is resolved first. + if err := cfg.EnsureResolved(); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + const numGoroutines = 10 + + // Launch multiple goroutines that all try to authenticate concurrently. + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + req, _ := http.NewRequest("GET", "https://test.com", nil) + // This should not panic or cause data races. + _ = cfg.Authenticate(req) + }() + } + + wg.Wait() + + // Verify authentication happened exactly once. + if cfg.credentialsProvider == nil { + t.Error("Expected credentialsProvider to be initialized") + } +} + +// TestAuthenticateIfNeededConcurrency tests authenticateIfNeeded directly. +func TestAuthenticateIfNeededConcurrency(t *testing.T) { + cfg := &Config{ + Host: "https://test.cloud.databricks.com", + Token: "fake-token", + } + + // Ensure config is resolved first. + if err := cfg.EnsureResolved(); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + const numGoroutines = 100 + + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _ = cfg.authenticateIfNeeded() + }() + } + + wg.Wait() + + // The important thing is that we didn't panic or hit data races. +} + +// TestAuthenticateOnce verifies that authentication happens exactly once +// even with concurrent calls. +func TestAuthenticateOnce(t *testing.T) { + callCount := 0 + var mu sync.Mutex + + cfg := &Config{ + Host: "https://test.cloud.databricks.com", + Credentials: &testStrategy{ + configure: func(ctx context.Context, c *Config) (credentials.CredentialsProvider, error) { + mu.Lock() + callCount++ + mu.Unlock() + return &testProvider{}, nil + }, + }, + } + + if err := cfg.EnsureResolved(); err != nil { + t.Fatal(err) + } + + var wg sync.WaitGroup + const numGoroutines = 50 + + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + _ = cfg.authenticateIfNeeded() + }() + } + + wg.Wait() + + mu.Lock() + count := callCount + mu.Unlock() + + if count != 1 { + t.Errorf("Expected Configure to be called exactly once, but was called %d times", count) + } +} + +// Test helpers. +type testStrategy struct { + configure func(ctx context.Context, c *Config) (credentials.CredentialsProvider, error) +} + +func (ts *testStrategy) Name() string { + return "test" +} + +func (ts *testStrategy) Configure(ctx context.Context, c *Config) (credentials.CredentialsProvider, error) { + if ts.configure != nil { + return ts.configure(ctx, c) + } + return &testProvider{}, nil +} + +type testProvider struct{} + +func (tp *testProvider) SetHeaders(r *http.Request) error { + r.Header.Set("Authorization", "Bearer test-token") + return nil +}