From a83b5f9fb20f1e6738f997b34657716e8d4adb94 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Fri, 30 May 2025 19:32:54 +0300 Subject: [PATCH 1/5] fix(manager): Improve channel closure handling - Updated IsClosed function to accurately check if a channel is closed without consuming data unless necessary. - Safely close the closedChan only if it is not already closed to avoid potential panics. --- internal/utils.go | 20 +++++++++++--------- manager/entraid_manager.go | 6 +++++- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/internal/utils.go b/internal/utils.go index 46b3842..a552657 100644 --- a/internal/utils.go +++ b/internal/utils.go @@ -1,16 +1,18 @@ package internal // IsClosed checks if a channel is closed. -// -// NOTE: It returns true if the channel is closed as well -// as if the channel is not empty. Used internally -// to check if the channel is closed. +// Returns true only if the channel is actually closed, not just if it has data available. +// +// WARNING: This function will consume one value from the channel if it has pending data. +// Use with caution on channels where consuming data might cause issues. func IsClosed(ch <-chan struct{}) bool { select { - case <-ch: - return true + case _, ok := <-ch: + // If ok is false, the channel is closed + // If ok is true, the channel had data (which we just consumed) + return !ok default: + // Channel is open but has no data available + return false } - - return false -} +} \ No newline at end of file diff --git a/manager/entraid_manager.go b/manager/entraid_manager.go index 3d633d2..21b2710 100644 --- a/manager/entraid_manager.go +++ b/manager/entraid_manager.go @@ -232,7 +232,11 @@ func (e *entraidTokenManager) stop() (err error) { } e.listener = nil - close(e.closedChan) + + // Safely close the channel - only close if not already closed + if !internal.IsClosed(e.closedChan) { + close(e.closedChan) + } return nil } From 95b59c546827870f224255b65669e019b0993352 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Fri, 30 May 2025 19:34:24 +0300 Subject: [PATCH 2/5] fix(manager): Add panic recovery in token manager goroutine - Implemented panic recovery in the Start method of entraidTokenManager to prevent crashes and ensure listener is notified of errors. --- internal/utils.go | 4 ++-- manager/entraid_manager.go | 11 +++++++++++ 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/internal/utils.go b/internal/utils.go index a552657..32b8912 100644 --- a/internal/utils.go +++ b/internal/utils.go @@ -2,7 +2,7 @@ package internal // IsClosed checks if a channel is closed. // Returns true only if the channel is actually closed, not just if it has data available. -// +// // WARNING: This function will consume one value from the channel if it has pending data. // Use with caution on channels where consuming data might cause issues. func IsClosed(ch <-chan struct{}) bool { @@ -15,4 +15,4 @@ func IsClosed(ch <-chan struct{}) bool { // Channel is open but has no data available return false } -} \ No newline at end of file +} diff --git a/manager/entraid_manager.go b/manager/entraid_manager.go index 21b2710..8accc70 100644 --- a/manager/entraid_manager.go +++ b/manager/entraid_manager.go @@ -148,6 +148,16 @@ func (e *entraidTokenManager) Start(listener TokenListener) (StopFunc, error) { e.listener = listener go func(listener TokenListener, closed <-chan struct{}) { + // Add panic recovery to prevent crashes + defer func() { + if r := recover(); r != nil { + // Attempt to notify listener of panic, but don't panic again if that fails + func() { + defer func() { _ = recover() }() + listener.OnError(fmt.Errorf("token manager goroutine panic: %v", r)) + }() + } + }() maxDelay := e.retryOptions.MaxDelay initialDelay := e.retryOptions.InitialDelay @@ -223,6 +233,7 @@ func (e *entraidTokenManager) stop() (err error) { err = fmt.Errorf("failed to stop token manager: %s", r) } }() + if e.ctxCancel != nil { e.ctxCancel() } From 776abb71e93fa713743e8141c21139cfa5a9d123 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Fri, 30 May 2025 19:34:42 +0300 Subject: [PATCH 3/5] fix(token): Enhance token creation logic and documentation - Updated the New function to return nil if expiresOn is zero to prevent panic. - Added logic to set receivedAt to the current time and recalculate TTL if receivedAt is zero. - Improved documentation to clarify the responsibilities of the caller regarding token validity and behavior when parameters are zero. --- token/token.go | 26 +++++++++++++++++++------- token/token_test.go | 11 ++++++----- 2 files changed, 25 insertions(+), 12 deletions(-) diff --git a/token/token.go b/token/token.go index 016ad2f..068eae0 100644 --- a/token/token.go +++ b/token/token.go @@ -10,12 +10,25 @@ import ( var _ auth.Credentials = (*Token)(nil) // New creates a new token with the specified username, password, raw token, expiration time, received at time, and time to live. -// NOTE: This won't do any validation on the token, expiresOn, receivedAt, or ttl. It will simply create a new token instance. -// The caller is responsible for ensuring the token is valid. +// NOTE: The caller is responsible for ensuring the token is valid. +// If the token is invalid, the behavior is undefined. +// - if expiresOn is zero, New returns nil +// - if receivedAt is zero, it will be set to the current time and TTL will be recalculated // Expiration time and TTL are used to determine when the token should be refreshed. // TTL is in milliseconds. // receivedAt + ttl should be within a millisecond of expiresOn +// If receivedAt is zero, it will be set to the current time and TTL will be recalculated. func New(username, password, rawToken string, expiresOn, receivedAt time.Time, ttl int64) *Token { + // If expiresOn is zero, return nil to avoid panic in ReceivedAt() + if expiresOn.IsZero() { + return nil + } + // If receivedAt is zero, set it to now and recalculate TTL to avoid race conditions in ReceivedAt() + if receivedAt.IsZero() { + receivedAt = time.Now() + ttl = expiresOn.Sub(receivedAt).Milliseconds() + } + return &Token{ username: username, password: password, @@ -28,6 +41,10 @@ func New(username, password, rawToken string, expiresOn, receivedAt time.Time, t // Token represents parsed authentication token used to access the Redis server. // It implements the auth.Credentials interface. +// +// WARNING: Use New() to create a new token. +// Creating a token with Token{} is invalid and will undefined behavior in the TokenManager. +// The zero value of Token is not valid. type Token struct { // username is the username of the user. username string @@ -60,11 +77,6 @@ func (t *Token) RawToken() string { // ReceivedAt returns the time when the token was received. func (t *Token) ReceivedAt() time.Time { - if t.receivedAt.IsZero() { - // set it to now, recalculate ttl - t.receivedAt = time.Now() - t.ttl = t.expiresOn.Sub(t.receivedAt).Milliseconds() - } return t.receivedAt } diff --git a/token/token_test.go b/token/token_test.go index 58134f5..54b1c99 100644 --- a/token/token_test.go +++ b/token/token_test.go @@ -94,14 +94,15 @@ func TestCopyToken(t *testing.T) { assert.NotEqual(t, token.expiresOn, copiedToken.expiresOn) // copy nil - copiedToken = copyToken(nil) - assert.Nil(t, copiedToken) + nilToken := copyToken(nil) + assert.Nil(t, nilToken) // copy empty token - copiedToken = copyToken(&Token{}) - assert.NotNil(t, copiedToken) + emptyToken := copyToken(&Token{}) + assert.Nil(t, emptyToken) anotherCopy := copiedToken.Copy() anotherCopy.rawToken = "changed" assert.NotEqual(t, copiedToken, anotherCopy) + assert.NotEqual(t, copiedToken.rawToken, anotherCopy.rawToken) } func TestTokenReceivedAt(t *testing.T) { @@ -124,7 +125,7 @@ func TestTokenReceivedAt(t *testing.T) { // Check if the copied token is a new instance assert.NotNil(t, tcopiedToken) - emptyRecievedAt := &Token{} + emptyRecievedAt := New("username", "password", "rawToken", time.Now(), time.Time{}, time.Hour.Milliseconds()) assert.True(t, emptyRecievedAt.ReceivedAt().After(time.Now().Add(-1*time.Hour))) assert.True(t, emptyRecievedAt.ReceivedAt().Before(time.Now().Add(1*time.Hour))) } From 790e68cc3d8cbb774664f5a5d59b9684b2847f97 Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov <1547186+ndyakov@users.noreply.github.com> Date: Sat, 31 May 2025 11:57:46 +0300 Subject: [PATCH 4/5] chore(token): remove some unnecessary comments --- token/token.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/token/token.go b/token/token.go index 068eae0..8e59a44 100644 --- a/token/token.go +++ b/token/token.go @@ -17,13 +17,10 @@ var _ auth.Credentials = (*Token)(nil) // Expiration time and TTL are used to determine when the token should be refreshed. // TTL is in milliseconds. // receivedAt + ttl should be within a millisecond of expiresOn -// If receivedAt is zero, it will be set to the current time and TTL will be recalculated. func New(username, password, rawToken string, expiresOn, receivedAt time.Time, ttl int64) *Token { - // If expiresOn is zero, return nil to avoid panic in ReceivedAt() if expiresOn.IsZero() { return nil } - // If receivedAt is zero, set it to now and recalculate TTL to avoid race conditions in ReceivedAt() if receivedAt.IsZero() { receivedAt = time.Now() ttl = expiresOn.Sub(receivedAt).Milliseconds() From d617d7b6e5f788dea35dd60999994184db36445b Mon Sep 17 00:00:00 2001 From: Nedyalko Dyakov Date: Mon, 2 Jun 2025 10:35:01 +0300 Subject: [PATCH 5/5] test(manager): change test delta to 5ms --- manager/entraid_manager_test.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/manager/entraid_manager_test.go b/manager/entraid_manager_test.go index 5cb7132..7ac8987 100644 --- a/manager/entraid_manager_test.go +++ b/manager/entraid_manager_test.go @@ -8,6 +8,8 @@ import ( "github.com/stretchr/testify/assert" ) +const testDurationDelta = float64(5 * time.Millisecond) + func TestDurationToRenewal(t *testing.T) { tests := []struct { name string @@ -236,7 +238,7 @@ func TestDurationToRenewal(t *testing.T) { } duration := manager.durationToRenewal(tt.token) - assert.InDelta(t, float64(tt.expectedDuration), float64(duration), float64(time.Millisecond), + assert.InDelta(t, float64(tt.expectedDuration), float64(duration), testDurationDelta, "%s: expected %v, got %v", tt.name, tt.expectedDuration, duration) }) } @@ -415,7 +417,7 @@ func TestDurationToRenewalMillisecondPrecision(t *testing.T) { } duration := manager.durationToRenewal(tt.token) - assert.InDelta(t, float64(tt.expectedDuration), float64(duration), float64(time.Millisecond), + assert.InDelta(t, float64(tt.expectedDuration), float64(duration), testDurationDelta, "%s: expected %v, got %v", tt.name, tt.expectedDuration, duration) }) } @@ -453,8 +455,8 @@ func TestDurationToRenewalConcurrent(t *testing.T) { if i == 0 { firstResult = result } else { - // All results should be within 10ms of each other - assert.InDelta(t, firstResult.Milliseconds(), result.Milliseconds(), 10) + // All results should be within 5ms of each other + assert.InDelta(t, firstResult.Milliseconds(), result.Milliseconds(), 5) } } }