-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix Importing Az in Parallel #18483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Importing Az in Parallel #18483
Conversation
df48745
to
87d1543
Compare
@@ -34,11 +33,10 @@ namespace Microsoft.Azure.Commands.Common.Authentication.Config | |||
internal class ConfigInitializer | |||
{ | |||
internal IDataStore DataStore { get; set; } = new DiskDataStore(); | |||
private static readonly object _fsLock = new object(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lock is no longer needed because the initialization is guarded by a mutex
@isra-fel is there an ETA on this release? I have a few customers asking on this fix release. Many thanks! |
Hi @jose-bueno-microsoft we are getting this reviewed and tested by different teams. Currently the ETA would be July 5th. |
… 18321-az-module-cannot-be-imported-in-parallel
… 18321-az-module-cannot-be-imported-in-parallel
… 18321-az-module-cannot-be-imported-in-parallel
var dataStore = settings?.DataStore; | ||
if (dataStore == null) | ||
{ | ||
dataStore = new MockDataStore(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why previous code didn't work? var dataStore = settings?.DataStore ?? new MockDataStore();
Description
Fix #18321
This PR resolves the file IO conflict when importing Az.Accounts in parallel PowerShell processes.
Root cause:
Config manager is initialized during importing Az.Accounts. It tries to create the config file if it does not exist. It also tries to write that file if there are old configs need to migrate. At the meantime, it reads the file to build config values.
All these file IO operations may conflict with each other. Causing Az.Accounts fail to be imported.
Fix:
Design:
This PR introduced a sibling to
ConfigManager
asSafeConfigManager
, which do not load configs from file system nor environment variables, so it's safe from unexpected exceptions.After that the name
ConfigManager
seemed inappropriate so I renamed it intoDefaultConfigManager
and introduced an abstract base classConfigManager
for both to inherit from.Now AzureSessionInitializer will first try to create a
DefaultConfigManager
and if it fails, useSafeConfigManager
instead.P.S. we might want to add this to smoke test.
Checklist
CONTRIBUTING.md
ChangeLog.md
file(s) has been updated:ChangeLog.md
file can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
## Upcoming Release
header -- no new version header should be added