Skip to content

Removing FileTokenCache.add overload. #27

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

Merged
merged 1 commit into from
Jul 16, 2019

Conversation

marstr
Copy link
Collaborator

@marstr marstr commented Jul 16, 2019

Fixes #26

@marstr marstr requested a review from rayluo July 16, 2019 20:27
@marstr
Copy link
Collaborator Author

marstr commented Jul 16, 2019

Pro: Tests-pass
Con: The lock is now scoped so that it isn't for the duration of this library's add operation.

I'm not sure how much of a con that really is, as at the end of the day the only trouble would be with cache merging semantics. I'm open to your interpretation of that!

Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks for identifying such a tricky issue! This quick fix looks reasonable, so I'm giving an approval right now, to allow a quick subsequent release if needed.

More thoughts:

  • Upstream repo does not have the same issue, because a re-entrant in-memory lock was used. In hindsight, this Extensions repo can potentially use a re-entrant lock provided by portalocker itself, but that seems like an undocumented feature, though.
  • We would definitely want to have a unit test case coverage on this, in near future, if not immediately in this PR. Currently the test cases in this repo are mostly skipped by default, due to the lack of testing accounts in existing CI. We should still be able to have some unit tests which does not rely on a real test account. Something like this.

@marstr marstr merged commit 71b5301 into AzureAD:dev Jul 16, 2019
@rayluo rayluo mentioned this pull request Jul 17, 2019
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.

Non re-entrant lock blocking extension use with msal>0.4.0
2 participants