[core-rest-pipeline] Idea for improving the tokenCycler

[sadasant]

This PR aims to do small improvements to the tokenCycler. The idea is that we could pick up the pace to do small improvements like this over time. Small refactorings like this should not change the behavior of our products, and can help us continue to improve how we express ideas as we spend more time working with them.

The tokenCycler was originally introduced to fix a bug in the @azure/core-http package. This package reached GA a while ago and since then it had some crucial components that had to be essentially replaced by the tokenCycler (they weren't publicly replaced, but internally they were replaced). As we wanted to fix the issue without introducing more public surface complexity, the tokenCycler became a bit its own ecosystem: No changes to the public API, and internally no reliance on existing APIs.

Later on we copied the cycler to core-rest-pipeline, and later as we introduced the challenge callbacks, it was necessary to expose some part of the cycler. Since the cycler exposes a getToken function (an async function that can receive scopes and getTokenOptions and eventually can return a token), this function became exposed to SDK client developers if they wanted to use the bearerTokenChallengePolicy. This public side of the cycler is very small, and I believe it is essentially perfect. It can't be made simpler, as it's a function that creates a getToken function.

Internally, the cycler has to deal with complexity related to:

• Actually retrieving the token, of course.
• How often to retry getting the token, and until when.
• When to wait or not wait for the token to be updated.

Some of these ideas could be eventually useful for other work we may do in the SDK. For now though, I believe doing a minimal set of changes to abstract away some of the functionality will reduce the complexity of the cycler by reducing the variable behavior. Business logic anywhere tends to feel like "this is just how it needs to be" (and thus may discourage changes), so as part of this PR, I'm attempting to make some of the functions the cycler uses more like universal utilities and less like subjective, variable business logic.

This PR:

• Reverts the useless AccessTokenRefresher interface and the associated cachedToken (I introduced this under the "CAE callbacks" PR, as an idea, then I forgot to clean this up) (I can do this in another PR if the rest of the changes aren't good for the team).
• Replaces the tryGetAccessToken inside the cycler's beginRefresh function with a generic tryUntilTimeout<T> which calls to any asyncFn until a given timeout is reached.
• Replaces the cycler's beginRefresh function with a generic retryUntilTimeout<T> which retries an asynchronous function as often as the provided interval milliseconds, until the provided Unix date timeout is reached.
• Renames the internal cycler object to cyclerState, as this object represents the most stateful elements of the cycler.

Reviews appreciated 🙇‍♂️🌞

[witemple-msft]

Overall I like the idea of taking the token cycler and making a robust abstraction out of it that can be used in many different ways. I have a few comments/concerns about the proposed changes, and at a very high level I think that if we want to make this a general purpose abstraction, that we would want to re-think a few aspects of the design.

[witemple-msft]

The issue I have with this is that it's still particular to the contract of getToken in core-auth. You wouldn't be able to use this abstraction with something that doesn't return null as a failure mode. The way this reads to me is "retry every X milliseconds as long as this function is returning null," and then in the inner function we catch errors and return them as null to retry, but that might not always be safe to do in the general case.

If we want to do this refactor with the intention of making this cycler abstraction easier to use with other types of functions, then I think we have to re-think the failure modes to come up with something truly generic. For example, what if the retryUntilTimeout function accepts as a parameter a predicate that can be used to determine whether an Error can be retried or should be treated as fatal?

[sadasant]

I agree that the failure modes are an issue on themselves and that null as a failure mode is not generic, but null as a failure mode is easy for two reasons:

• Has way less overhead.
• It's as clear as possible on the type that uses null as the failure mode. It has been clear enough for us to use it in the TokenCredential, for example 🤔

As an alternative to null as a failure mode, what would be better while still not make it much more complicated?

[witemple-msft]

To me the names tryUntilTimeout and retryUntilTimeout mean the same thing. This function is specifically made to be part of the inner loop of retryUntilTimeout, and I think it has limited utility on its own.

[sadasant]

Inside of retryUntilTimeout I think it adds too much complexity for one function. The name could be better though. This could become something like "run silently unless a condition is met", and receive a condition instead of a timeout, which would be generic.

[witemple-msft]

I just think it's a very particular function. "tryOnceAndThrowIfTimeoutExpired"... It only makes sense to be used with retryUntilTimeout, which still parses in my brain as meaning the exact same thing as tryUntilTimeout. Maybe this function could simply be called tryOnce.

I generally like the idea of accepting a predicate representing the error condition instead of checking the date, but like I mentioned in the other comment, I'm skeptical that this makes much sense to use with other "refreshable things" as long as | null is baked so deeply in. That feels like a leaky abstraction where things would start wanting to return null as a failure mode just for the sake of fitting into that abstraction, even if it's more natural to reject/call a handler, etc.

It might also be possible to use some clever promise chains to eliminate this secondary function entirely.

[sadasant]

Oh I see! I like that. Alright, let's come back to this later when we have more time 👍

[voidfoo]

Related #15678

[ghost]

Hi @sadasant. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.