EdgeHub: Fix TokenUpdate logic #206
Conversation
| TaskCompletionSource<string> tcs = this.tokenGetter | ||
| .GetOrElse( | ||
| () => | ||
| // We have to catch UnauthorizedAccessException, because on IsTokenUsable, we call parse from |
There was a problem hiding this comment.
Where does this catch occur?
There was a problem hiding this comment.
In GetTokenExpiry (and IsTokenUsable also catches all exceptions)
| @@ -263,33 +264,55 @@ void InternalConnectionStatusChangesHandler(ConnectionStatus status, ConnectionS | |||
| async Task<string> GetNewToken(string iotHub, string id, string currentToken, IIdentity currentIdentity) | |||
There was a problem hiding this comment.
Could you please add some comments explaining the retry logic.
|
|
||
| retrying = true; |
There was a problem hiding this comment.
Are we iterating the loop again only to log a message? Could we log the NewTokenObtained event here and return the new token right away?
There was a problem hiding this comment.
We could, but wanted to avoid the duplicate code. This seemed cleaner :)
|
|
||
| if (newTokenGetterCreated) | ||
| { | ||
| if (retrying) |
There was a problem hiding this comment.
It is not clear to me when and how many times 'newTokenGetterCreated' and 'retrying' would both be true. Perhaps some comments could help clarify when tokenGetters are set and cleared.
| this.tokenGetter = Option.None<TaskCompletionSource<string>>(); | ||
| Events.NewTokenObtained(newCredentials.Identity.IotHubHostName, newCredentials.Identity.Id, tokenAuth.Token); | ||
| tg.SetResult(tokenAuth.Token); |
There was a problem hiding this comment.
Because the code below expects that the tokenGetter will be reset when the value is set. And that also answers your other question, I suppose
There was a problem hiding this comment.
In principle, if order is important, then we should do these under a lock. Otherwise order shouldn't matter.
There was a problem hiding this comment.
As discussed offline, the order is important because the caller is waiting for SetResult to be called (not looping), and when it resumes, it expects this.tokenGetter to be reset.
| return taskCompletionSource; | ||
| }); | ||
|
|
||
| // If a new tokenGetter was created, then invoke the connection status changed handler |
There was a problem hiding this comment.
It is still not clear to me from the comments when a new tokenGetter is created. That is key to understanding this loop.
There was a problem hiding this comment.
Given the lack of general documentation in our code base, having comments that explain not just things local to a piece of code, but spanning components of code, would be very helpful.
There was a problem hiding this comment.
Yes, I agree with you. I am just a bit weary of comments getting stale in that case...
When getting a new token to send on the CBS link, if a soon expiring token is received, instead of returning it, try to get a fresh token. This allows EdgeHub to wait for the device to send a new token, instead of going into token refresh loop.
This PR also fixes this issue - #182