Properly handle cancelled-by-user response in HttpSource #388
Conversation
|
This looks like a good fix. Is there any reason the UI prompt doesn't handle this exception itself? |
|
|
||
| // null means cancelled by user | ||
| // block subsequent attempts to annoy user with prompts | ||
| _authRetries = HttpHandlerResourceV3Provider.MaxAuthRetries; |
There was a problem hiding this comment.
Why is _authRetries a global field of for this source, instead of a counter just for the current request?
There was a problem hiding this comment.
During an install this would end up coming up constantly. For different UI actions it may make sense to retry for each. Do you have a specific scenario where a user might be unable to provide credentials and have to continue on but later have the correct password?
There was a problem hiding this comment.
The motivation behind the fix was addressing a regression from 3.3:
- 3 retries of credential prompts when user hits cancel
- Failing to authenticate means restarting visual studio, given that vsts plans to deprecate credentials rather fast, I don’t think that is acceptable
VS credential dialog throws OperationCanceledException when user hits cancel. Added special catch to reset authentication state of HttpSource retry loop. After the change "cancelled" source will be blocked for following authentication attempts.
The purpose of this change is to scope source authentication state to an abstract activity. - Introduced an ambient context class with a single property 'CorrelationId' identifying current activity - HttpSource makes use of the CorrelationId by mapping it to a AuthenticationState, e.g. retries counter, blocking state. - Added a call to create new activity in key places such as install package, uninstall package, restore missing packages, start search
alpaix
force-pushed
the
alpanov/auth-bug
branch
from
a16b561
to
a3156b5
Compare
|
//ping @rrelyea @emgarten @deepakaravindr @joelverhagen |
|
Filed a NuGet/Home#2362 describing the actual problem as reported in email. |
|
What's the status here? |
|
@joelverhagen The fix wasn't approved for 3.4.xxx release. As these milestones are all behind us I'm going to merge it to dev only. BTW, any feedback on the fix itself? |
| /// <summary> | ||
| /// Represents activity unique ID. | ||
| /// </summary> | ||
| public string CorrelationId { get; private set; } |
There was a problem hiding this comment.
Why a string? Guid is smaller :).
There was a problem hiding this comment.
String is generic universal representation of id that can be anything. I didn't want to limit it to a specific type. Memory footprint difference is not significant here.
|
⌚ |
VS credential dialog throws OperationCanceledException when user hits cancel.
Added special catch to reset authentication state of HttpSource retry loop.
After the change "cancelled" source will be blocked for following authentication attempts.
//cc @yishaigalatzer @emgarten @deepakaravindr