Skip to content
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

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from
Open

Make portalocker optional #117

wants to merge 1 commit into from

Conversation

rayluo
Copy link
Contributor

@rayluo rayluo commented Oct 20, 2022

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:

  • A lock mechanism is only used to guard against garbled data due to concurrent read/write. A lock mechanism in itself does not make the higher level application more or less performant. When your app is overloaded, previously (i.e. before this PR) you will see portalocker.exceptions.LockException which means the locking mechanism is doing its job. After this PR, under the same heavy load, you will see LockError 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

@rayluo rayluo force-pushed the optional-portalocker branch 5 times, most recently from 7ac2952 to 323ac39 Compare October 20, 2022 07:20
Comment on lines +29 to +30
- python-version: "3.10"
toxenv: "py310"
Copy link
Contributor

Choose a reason for hiding this comment

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

Python 3.11 is GA. We are planning to support it in next sprint (Azure/azure-cli#24109).

"""It will be raised when unable to obtain a lock"""


class CrossPlatLock(object):
Copy link
Contributor

@jiasli jiasli Oct 27, 2022

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.

Copy link
Contributor Author

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.

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.

Consider removing portalocker dependency
2 participants