feat: adding semaphore locking for PersistedAccessTokenCache #667
Conversation
|
tanel.tinits seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
Thanks! I really appreciate your contribution, please sign our CLA otherwise I can't merge it. Furthermore, I added some comments, maybe we can split up the PR to fix these different issues concurrency and TTL separately.
Did you tried whether it solves your issues you have observed?
| @@ -70,12 +81,26 @@ private async Task<string> GetValidToken(string audience, AccessToken currentAcc | |||
| { | |||
| var now = DateTimeOffset.UtcNow.ToUnixTimeMilliseconds(); | |||
| var dueDate = currentAccessToken.DueDate; | |||
| if (now < dueDate) | |||
|
|
|||
| if (now < dueDate - 15000) // Adding 15 sec buffer to update the token before it really expires. | |||
There was a problem hiding this comment.
Lets focus on one topic/part in this PR. We can do/fix this with #642
| @@ -86,7 +111,17 @@ private async Task<AccessToken> FetchNewAccessToken(string audience) | |||
| { | |||
| var newAccessToken = await accessTokenFetcherAsync(); | |||
| CachedCredentials[audience] = newAccessToken; | |||
| WriteCredentials(); | |||
|
|
|||
| await semaphore.WaitAsync(); | |||
There was a problem hiding this comment.
❓ I guess we could use just lock here right? Any specifics/benefits about the semphore here (as you also use just one count)? Please be aware coming from the java world, so I might not be aware of some language specifics from c#.
There was a problem hiding this comment.
I will check if I can use just a lock. imo I can block the thread and wait until its available with lock. But in current case I want to check from another thread if the lock is in place and then it would not wait to fetch another token. Rather it could use existing token if its still valid.
There is some Monitor that is using lock behind the scene.
I will mark this PR as draft for now and come back with the updates.
There was a problem hiding this comment.
Thanks that makes sense 👍
|
Managed to solve the issue by using another token supplier for the zb-client. Not using the CamundaCloudTokenProvider. |
closes #