Skip to content

Conversation

@msJinLei
Copy link
Contributor

@msJinLei msJinLei commented Feb 22, 2022

Request are used after disposed in AuthenticationHelper and so the previous request cannot be disposed.
The new request should be disposed finally.

Description

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
      • {Please put the link here}
    • the markdown help files have been regenerated using the commands listed here

@msJinLei msJinLei requested review from dingmeng-xue, dolauli and isra-fel and removed request for dingmeng-xue February 22, 2022 03:12
isra-fel
isra-fel previously approved these changes Feb 22, 2022
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Have you tested CAE scenario?

if (!string.IsNullOrEmpty(claimsChallenge))
{
await processor.OnClaimsChallenageAsync(newRequest, claimsChallenge, cancelToken).ConfigureAwait(false);
response = await next(newRequest, cancelToken, cancelAction, signal);
Copy link
Contributor

@dolauli dolauli Feb 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double check the code and think we need to dispose the old response here.

@msJinLei msJinLei added this to the Feb 2022 (2022-03-01) milestone Feb 22, 2022
@msJinLei msJinLei changed the base branch from main to release-2022-03-01 February 22, 2022 07:50
@msJinLei msJinLei dismissed isra-fel’s stale review February 22, 2022 07:50

The base branch was changed.

@msJinLei msJinLei requested review from dolauli and isra-fel February 22, 2022 08:59
isra-fel
isra-fel previously approved these changes Feb 22, 2022
Copy link
Member

@isra-fel isra-fel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Lei Jin and others added 2 commits February 22, 2022 17:35
Request are used after disposed in AuthenticationHelper and so the
previous request cannot be disposed.
The new request should be disposed finally.
var response = await next(request, cancelToken, cancelAction, signal);

if (response.MatchClaimsChallengePattern())
using (var newRequest = await request.CloneWithContent(request.RequestUri, request.Method))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
using (var newRequest = await request.CloneWithContent(request.RequestUri, request.Method))
using (var newRequest = await request.CloneWithContent(request.RequestUri, request.Method))
using (var response = await next(request, cancelToken, cancelAction, signal))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refer to the implementation of pipeline, the original request and response will be disposed finally.

@dolauli dolauli merged commit e4fd7ab into release-2022-03-01 Feb 23, 2022
@msJinLei msJinLei deleted the jinlei/cae_fix branch January 12, 2023 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants