Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] Fix non thread safe token cache behavior #5387

Closed
2 tasks done
pmahindrakar-oss opened this issue May 17, 2024 · 3 comments
Closed
2 tasks done

[BUG] Fix non thread safe token cache behavior #5387

pmahindrakar-oss opened this issue May 17, 2024 · 3 comments
Assignees
Labels
bug Something isn't working

Comments

@pmahindrakar-oss
Copy link
Contributor

Describe the bug

Currently the client auth interceptor used by multiple flyte services faces an issue when multiple parallel attempts are made to get token as the underlying token source implementation is not thread safe

https://github.com/flyteorg/flyte/blob/master/flyteidl/clients/go/admin/auth_interceptor.go#L145

Eg : https://github.com/flyteorg/flyte/blob/master/flyteidl/clients/go/admin/cache/token_cache_inmemory.go

Expected behavior

The implementation of token cache should be thread safe

Additional context to reproduce

No response

Screenshots

No response

Are you sure this issue hasn't been raised already?

  • Yes

Have you read the Code of Conduct?

  • Yes
@pmahindrakar-oss pmahindrakar-oss added bug Something isn't working untriaged This issues has not yet been looked at by the Maintainers labels May 17, 2024
Copy link

runllm bot commented May 17, 2024

The current implementation of the token cache in Flyte is not thread-safe, which can cause issues when multiple parallel attempts are made to get a token. This is evident in the TokenCacheInMemoryProvider class, which does not use any synchronization mechanisms to ensure thread safety.

To make the token cache thread-safe, you can use synchronization primitives like mutexes. Here's an example of how you can modify the TokenCacheInMemoryProvider to be thread-safe:

package cache

import (
	"fmt"
	"sync"

	"golang.org/x/oauth2"
)

type TokenCacheInMemoryProvider struct {
	token *oauth2.Token
	mu    sync.Mutex
}

func (t *TokenCacheInMemoryProvider) SaveToken(token *oauth2.Token) error {
	t.mu.Lock()
	defer t.mu.Unlock()
	t.token = token
	return nil
}

func (t *TokenCacheInMemoryProvider) GetToken() (*oauth2.Token, error) {
	t.mu.Lock()
	defer t.mu.Unlock()
	if t.token == nil {
		return nil, fmt.Errorf("cannot find token in cache")
	}
	return t.token, nil
}

Would you like more details on this implementation?

Sources:


This is an AI-generated response and your feedback is appreciated! Please leave a 👍 if this is helpful and 👎 if it is not.

Mention @RunLLM to ask follow-up questions. See runllm.com for more!

@pmahindrakar-oss pmahindrakar-oss self-assigned this May 20, 2024
@eapolinario eapolinario removed the untriaged This issues has not yet been looked at by the Maintainers label May 23, 2024
@pingsutw
Copy link
Member

@pmahindrakar-oss, does #5407 fix it? If so, could we close the issue?

@pmahindrakar-oss
Copy link
Contributor Author

Yes we can close this issue

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants