Skip to content

first cut at proper read/write locking for KeyBundle #83

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 5 commits into from
Apr 12, 2021

Conversation

jschlyter
Copy link
Collaborator

KeyBundle is tricky as it updates its own keys from multiple places in the code. This PR uses readwriterlock to put a read lock around key reads and a write lock updates.

The write lock is implemented using a decorator, whereas the read lock implemented using explicit with. Most key access has been moved to keys() in order to simplify the code.

There may be dragons - please review carefully. Extra set of eyes from @janste63 would be most useful.

@jschlyter jschlyter requested a review from rohe April 7, 2021 17:20
@jschlyter jschlyter self-assigned this Apr 7, 2021
@codecov-io
Copy link

codecov-io commented Apr 7, 2021

Codecov Report

Merging #83 (9656a55) into develop (c9e59bc) will increase coverage by 0.10%.
The diff coverage is 95.23%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop      #83      +/-   ##
===========================================
+ Coverage    77.50%   77.60%   +0.10%     
===========================================
  Files           41       41              
  Lines         4276     4296      +20     
  Branches       832      833       +1     
===========================================
+ Hits          3314     3334      +20     
  Misses         700      700              
  Partials       262      262              
Impacted Files Coverage Δ
src/cryptojwt/key_bundle.py 79.52% <95.23%> (+0.59%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c9e59bc...9656a55. Read the comment docs.

Copy link
Contributor

@janste63 janste63 left a comment

Choose a reason for hiding this comment

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

Missed writer method: do_keys
Missed reader methods: contains, iter, len, copy
Maybe iter should work on a copy to avoid errors.

_typs = [typ.lower(), typ.upper()]
_keys = [k for k in self._keys if k.kty in _typs]
else:
_keys = self._keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a shallow copy here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if update:
self._uptodate()
with self._lock_reader:
return self._keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Need a shallow copy here. Optimization, save the copy and remove copy every time keys_writer() is called.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@jschlyter
Copy link
Collaborator Author

Missed writer method: do_keys

Not needed as update is wrapped. If one wraps do_keys as well you have a conflict.

Missed reader methods: contains, iter, len, copy

Fixed via caead3f

Maybe iter should work on a copy to avoid errors.

670d0bc

@janste63
Copy link
Contributor

Missed writer method: do_keys

Not needed as update is wrapped. If one wraps do_keys as well you have a conflict.

Well since the name doesn't start with "_" it suggest that it can be called from the outside. Suggestion, make it private.

The method load() calls do_keys() with no locking.

@jschlyter jschlyter merged commit cdc11da into IdentityPython:develop Apr 12, 2021
@jschlyter jschlyter deleted the more_keybundle_lock branch April 12, 2021 15:25
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.

4 participants