[Core HTTP] Bug Fix - TokenRefresher shouldn't be recreated for each request #10728

HarshaNalluru · 2020-08-20T20:47:26Z

Summary of the investigation

History

I was trying to fix the CI for the schema-registry PR(#10602) which was failing because the recordings were not generated using the latest master. Upon digging deeper #10683, the failure was caused because the tests weren't recorded using the latest core-http(on master) which had a new change #10085 related to the token refreshing logic that made more requests to refresh the tokens that weren't saved in the older recordings.

Bug 1 (Core HTTP)

New requests mentioned above led to a discussion with @sadasant and upon investigating further, we realized that the tokens weren't cached which caused more requests. Daniel made a PR #10692 to fix the issue to update core-http with caching the tokens that would remove the unnecessary additional requests.

Bug 2 (Core HTTP)

After fixing Bug 1, we generated the recordings, played them back, and have observed one more issue where there was a refresh request that was happening after the test ends. #10694
Another question at this point is that the refresh request is supposed to ping after or at about 30s, why does it ping again after the test when it had just elapsed a couple of seconds(milliseconds if in playback)?
Upon investigating with Daniel to answer the above, the issue turned out to be that a new token refresher was being created for each of the requests that are made to the service.
This PR attempts to fix that by reusing the token refresher.

Many thanks to @sadasant for his help with investigating the issues.

Resolves #10694

HarshaNalluru · 2020-08-20T21:12:17Z

.../aad_formrecognizerclient_nodejs_only/recording_recognizes_content_from_a_pdf_file_stream.js

-  'Tue, 18 Aug 2020 23:52:08 GMT'
-]);
-
-nock('https://login.microsoftonline.com:443', {"encodedQueryParams":true})


This deletion is an example of the request that was saved because the token refresher wasn't shared.

…cationPolicy

xirzec

Looks good!

xirzec · 2020-08-20T22:32:35Z

sdk/core/core-http/review/core-http.api.md

+// @public
+export class AccessTokenRefresher {
+    constructor(credential: TokenCredential, scopes: string | string[], requiredMillisecondsBeforeNewRefresh?: number);
+    // (undocumented)


missing doc comment

Thanks! Fixed!

I see more

I'll log an issue for this.

HarshaNalluru · 2020-08-20T22:39:05Z

sdk/core/core-http/review/core-http.api.md

@@ -100,7 +108,7 @@ export class BasicAuthenticationCredentials implements ServiceClientCredentials

 // @public
 export class BearerTokenAuthenticationPolicy extends BaseRequestPolicy {
-    constructor(nextPolicy: RequestPolicy, options: RequestPolicyOptions, credential: TokenCredential, scopes: string | string[], tokenCache: AccessTokenCache);
+    constructor(nextPolicy: RequestPolicy, options: RequestPolicyOptions, tokenCache: AccessTokenCache, tokenRefresher: AccessTokenRefresher);


Would this be a breaking change?

This might be fine because the libraries depend on BearerTokenAuthenticationPolicy indirectly through bearerTokenAuthenticationPolicy which is not being changed.

daviwil

Looks fine, might be worth mentioning this change in the CHANGELOG before the next release just in case anyone happens to be using it directly (which they shouldn't). @xirzec feel free to say otherwise, I don't feel strongly about it

sdk/core/core-http/src/credentials/accessTokenRefresher.ts

sadasant

Yayyy thank you

…hub.com/HarshaNalluru/azure-sdk-for-js into harshan/recorder/issue/delayed-requests

HarshaNalluru · 2020-08-21T00:06:56Z

sdk/core/core-http/CHANGELOG.md

+- `BearerTokenAuthenticationPolicy` constructor signature has been updated to accommodate the token refresher appropriately. The signature for `bearerTokenAuthenticationPolicy` method doesn't change.
+  [PR 10728](https://github.com/Azure/azure-sdk-for-js/pull/10728)
+  Note: It is expected that the users depending on `BearerTokenAuthenticationPolicy` do so indirectly through `bearerTokenAuthenticationPolicy` method that is exported.


@sadasant, feel free to update the changelog with more details spanning #10085 and #10692

@daviwil @xirzec - Is this fine?

Sounds reasonable to me, thanks!

xirzec · 2020-08-21T00:12:48Z

sdk/core/core-http/CHANGELOG.md

+- `BearerTokenAuthenticationPolicy` constructor signature has been updated to accommodate the token refresher appropriately. The signature for `bearerTokenAuthenticationPolicy` method doesn't change.
+  [PR 10728](https://github.com/Azure/azure-sdk-for-js/pull/10728)
+  Note: It is expected that the users depending on `BearerTokenAuthenticationPolicy` do so indirectly through `bearerTokenAuthenticationPolicy` method that is exported.


This feels like a lot of context/details for a change to what is effectively an internal type.

Suggested change

- `BearerTokenAuthenticationPolicy` constructor signature has been updated to accommodate the token refresher appropriately. The signature for `bearerTokenAuthenticationPolicy` method doesn't change.

[PR 10728](https://github.com/Azure/azure-sdk-for-js/pull/10728)

Note: It is expected that the users depending on `BearerTokenAuthenticationPolicy` do so indirectly through `bearerTokenAuthenticationPolicy` method that is exported.

- Fixed a bug in `BearerTokenAuthenticationPolicy` where a helper class was being instantiated on each request. [PR 10728](https://github.com/Azure/azure-sdk-for-js/pull/10728)

I think this is enough honestly. Nobody should have a reason to grab the per-instance class and mess with it. Alternatively we could call this out as 'BREAKING' but honestly BearerTokenAuthenticationPolicy should be private.

I don't really understand why we exported that type, since it's not used as the return type of bearerTokenAuthenticationPolicy -- other policy classes aren't exported e.g.

export { LogPolicyOptions, logPolicy } from "./policies/logPolicy";

But some recent ones like DisableResponseDecompressionPolicy are, which just feels like a bug. Maybe this was done to make tests easier to write?

I don't really understand why we exported that type, since it's not used as the return type of bearerTokenAuthenticationPolicy -- other policy classes aren't exported

Hmmm, they shouldn't have been exported.

But some recent ones like DisableResponseDecompressionPolicy are, which just feels like a bug.

Looks like KeepAlivePolicy is being exported too.
export { keepAlivePolicy, KeepAlivePolicy, KeepAliveOptions } from "./policies/keepAlivePolicy";

Maybe this was done to make tests easier to write?

We import the src code directly inside the tests, no need to export it through index.ts(or coreHttp.ts) just for the sake of tests.

Opened a PR #10738 to un-export these classes.

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

sadasant · 2020-08-21T10:14:43Z

This solves the problem and it's neatly done. Ship it!

HarshaNalluru · 2020-08-21T17:56:25Z

This solves the problem and it's neatly done. Ship it!

Couldn't have done it without you!!

HarshaNalluru added 2 commits August 19, 2020 23:12

console logs in core-http

e8fbec8

draft 2 - fix to share the tokenRefresher

e621b25

HarshaNalluru commented Aug 20, 2020

View reviewed changes

HarshaNalluru added 4 commits August 20, 2020 14:19

remove console.logs meants for debugging

5c2bb31

remove separate tokenRefresher declaration inside BearerTokenAuthenti…

57a680e

…cationPolicy

remove console.logs in Form recognizer meant for debugging

e9970ee

fix build

4fafc7f

HarshaNalluru marked this pull request as ready for review August 20, 2020 22:18

HarshaNalluru requested review from bterlson, chradek, daviwil, jeremymeng, willmtemple and xirzec as code owners August 20, 2020 22:18

xirzec approved these changes Aug 20, 2020

View reviewed changes

HarshaNalluru commented Aug 20, 2020

View reviewed changes

daviwil approved these changes Aug 20, 2020

View reviewed changes

sdk/core/core-http/src/credentials/accessTokenRefresher.ts Outdated Show resolved Hide resolved

sadasant approved these changes Aug 20, 2020

View reviewed changes

HarshaNalluru added 7 commits August 20, 2020 15:47

Update sdk/core/core-http/src/credentials/accessTokenRefresher.ts

94f7ac5

Update sdk/core/core-http/src/credentials/accessTokenRefresher.ts

68fe2fc

Docs and remove new line

316308d

API Report

4569eed

Merge branch 'harshan/recorder/issue/delayed-requests' of https://git…

bb6a5af

…hub.com/HarshaNalluru/azure-sdk-for-js into harshan/recorder/issue/delayed-requests

changelog

d07f31d

changelog - minor update

4f98009

HarshaNalluru commented Aug 21, 2020

View reviewed changes

xirzec reviewed Aug 21, 2020

View reviewed changes

Update sdk/core/core-http/CHANGELOG.md

216b0df

Co-authored-by: Jeff Fisher <xirzec@xirzec.com>

This was referenced Aug 21, 2020

[Core HTTP] Un-export {...}Policy classes that were meant to be internal only #10738

Merged

[Core HTTP] Token refresher is not shared among multiple requests #10694

Closed

HarshaNalluru merged commit d752581 into Azure:master Aug 21, 2020

jeremymeng mentioned this pull request Sep 2, 2020

[Core] Update changelog date for Sep. 2020 release #10972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core HTTP] Bug Fix - TokenRefresher shouldn't be recreated for each request #10728

[Core HTTP] Bug Fix - TokenRefresher shouldn't be recreated for each request #10728

HarshaNalluru commented Aug 20, 2020 •

edited

HarshaNalluru Aug 20, 2020

xirzec left a comment

xirzec Aug 20, 2020

HarshaNalluru Aug 20, 2020 •

edited

HarshaNalluru Aug 20, 2020

HarshaNalluru Aug 20, 2020

HarshaNalluru Aug 20, 2020

daviwil left a comment

sadasant left a comment

HarshaNalluru Aug 21, 2020

daviwil Aug 21, 2020

xirzec Aug 21, 2020

HarshaNalluru Aug 21, 2020

HarshaNalluru Aug 21, 2020 •

edited

sadasant commented Aug 21, 2020

HarshaNalluru commented Aug 21, 2020

[Core HTTP] Bug Fix - TokenRefresher shouldn't be recreated for each request #10728

[Core HTTP] Bug Fix - TokenRefresher shouldn't be recreated for each request #10728

Conversation

HarshaNalluru commented Aug 20, 2020 • edited

Summary of the investigation

History

Bug 1 (Core HTTP)

Bug 2 (Core HTTP)

Choose a reason for hiding this comment

xirzec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Aug 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviwil left a comment

Choose a reason for hiding this comment

sadasant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarshaNalluru Aug 21, 2020 • edited

Choose a reason for hiding this comment

sadasant commented Aug 21, 2020

HarshaNalluru commented Aug 21, 2020

HarshaNalluru commented Aug 20, 2020 •

edited

HarshaNalluru Aug 20, 2020 •

edited

HarshaNalluru Aug 21, 2020 •

edited