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

core: added mutex lock #23

Merged
merged 4 commits into from
Aug 30, 2023

Conversation

alejandromumo
Copy link
Member

@alejandromumo alejandromumo commented Aug 11, 2023

@alejandromumo alejandromumo marked this pull request as ready for review August 14, 2023 10:48
@alejandromumo
Copy link
Member Author

alejandromumo commented Aug 14, 2023

It seems that tests are not enabled in the CI. I ran them locally:


---------- coverage: platform darwin, python 3.9.16-final-0 ----------
Name                          Stmts   Miss  Cover   Missing
-----------------------------------------------------------
invenio_cache/__init__.py         8      0   100%
invenio_cache/_compat.py          5      0   100%
invenio_cache/bccache.py          8      0   100%
invenio_cache/config.py           6      0   100%
invenio_cache/decorators.py      31      0   100%
invenio_cache/errors.py           8      1    88%   21
invenio_cache/ext.py             31      0   100%
invenio_cache/lock.py            67      4    94%   72, 80, 84, 88
invenio_cache/proxies.py          7      0   100%
-----------------------------------------------------------
TOTAL                           171      5    97%

======================================================================================================================================== 17 passed, 9 skipped, 14 warnings in 0.40s ========================================================================================================================================

Uncovered lines for lock.py are just a property and three base methods that raise NotImplementedError

For errors.py it is a __str__ method that is not being tested.

Copy link
Contributor

@ntarocco ntarocco left a comment

Choose a reason for hiding this comment

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

I have re-enabled the CI tests

invenio_cache/decorators.py Outdated Show resolved Hide resolved
Comment on lines 136 to 137
if self.acquired:
success = self._cache.delete(self.lock_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the lock was not acquired for some reasons, we might want to return True or to fail:

Suggested change
if self.acquired:
success = self._cache.delete(self.lock_id)
if self.acquired:
success = self._cache.delete(self.lock_id)
else:
success = True

Otherwise, it might return False and you don't know what happened.
WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Re-iterating on this: I would remove all if self.aquired. You need to be sure to pass the CachedMutex instance to release. I would rely exclusively on the existence of the cache key instead.
What is the use case? Multiprocessing context?

My worry is the following: if improperly used, it might happen that I acquire in one process (one web node), and try to release on another process (another web node) later one. This will fail, unless the same instance of CachedMutex is shared.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with that.

However, the lock might be released by any thread right now.

E.g.

# Thread 1
lock = Mutex("id1")
lock.acquire()

# Thread 2
lock = Mutex("id1")
lock.release()

We could "sign" these locks with a unique ID (e.g. set the value to a UUID) and only allow the release / renewal if the thread passes the same uuid. We briefly talked about it but did not elaborate on it.

@zzacharo zzacharo merged commit aebb80c into inveniosoftware:master Aug 30, 2023
3 checks passed
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.

[USER MODERATION] - 4th deliverable - lock moderation actions
3 participants