-
Notifications
You must be signed in to change notification settings - Fork 29
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
Make portalocker optional #117
base: dev
Are you sure you want to change the base?
Conversation
7ac2952
to
323ac39
Compare
"""It will be raised when unable to obtain a lock""" | ||
|
||
|
||
class CrossPlatLock(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.
Well, Azure CLI doesn't really need such a complex lock at the comment. Previously ADAL didn't have locking mechanism and worked mostly fine.
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.
Fair point. But if the current PR's non-portalocker lock is harmless, then we might as well just use it.
We are open to the dummy lock idea, too. We may work on it at a later time. It can be done, but it is more work, in terms of needing to expand the current MSAL EX api to allow a "no lock" option. (The current file-based lock, on the contrary, is a drop-in replacement for portalocker, and keeps the MSAL EX api intact.)
If the file-based lock does not work well for Azure CLI on SAW, we will prioritize the dummy lock work accordingly.
323ac39
to
74812a4
Compare
See this comment on how to test this PR. |
74812a4
to
beb65a2
Compare
This is currently an experimental PR. It will not be merged, not until we receive some test feedback. To beta test this PR, you will need to pull it from its feature branch.
Usage: There is no API change. MSAL EX will automatically utilize portalocker when portalocker is available in current environment, and switch to a new portalocker-less implementation otherwise.
Note:
portalocker.exceptions.LockException
which means the locking mechanism is doing its job. After this PR, under the same heavy load, you will seeLockError
instead, which also means the new lock mechanism is functioning.Test automation currently fails, because keyring daemon no longer works. It used to work even in headless test environment. We will look into it at a later time. But this should not affect or block the beta testing of this PR.CC: @jiasli