Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 18 additions & 9 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is a behaviour change from the current implementation. If the credential providers returned an error earlier, it would set c.credentialsProvider to nil, and the authenticateIfNeeded function could be called again. Now, it is no longer an option as the sync.Once function will run exactly once.

})
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{}
}
Expand Down
140 changes: 140 additions & 0 deletions config/config_concurrency_test.go
Original file line number Diff line number Diff line change
@@ -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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This verifies that authentication happened atleast once, right?

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 {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy path should ideally not be branched

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
}
Loading