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

BF: do not use .acquired - just get state from acquire() #5718

Merged
merged 2 commits into from Jun 10, 2021

Conversation

yarikoptic
Copy link
Member

Somewhat ad-hoc and makes an already confusing lock_if_check_fails even more so
by optionally changing its return signature. So eventually we might want
to RF this whole piece of code. But I think it should work ok for now.
Anyways -- fasteners 0.16.x series might re-gain .acquired (see
harlowja/fasteners#71) and we might want to see to RF
this PR properly if so instead of merging/releasing it right away. But FWIW
Closes #5717

Somewhat ad-hoc and makes an already confusing lock_if_check_fails even more so
by optionally changing its return signature. So eventually we might want
to RF this whole piece of code.  But I think it should work ok for now.
Anyways -- fasteners 0.16.x series might re-gain .acquired (see
harlowja/fasteners#71) and we might want to see to RF
this PR properly if so instead of merging/releasing it right away.  But FWIW
Closes datalad#5717
@codecov
Copy link

codecov bot commented Jun 7, 2021

Codecov Report

Merging #5718 (5d51da1) into maint (a1a2106) will decrease coverage by 60.76%.
The diff coverage is 50.00%.

❗ Current head 5d51da1 differs from pull request most recent head cc180b8. Consider uploading reports for the commit cc180b8 to get more accurate results
Impacted file tree graph

@@             Coverage Diff             @@
##            maint    #5718       +/-   ##
===========================================
- Coverage   90.28%   29.52%   -60.77%     
===========================================
  Files         299      296        -3     
  Lines       42349    42316       -33     
===========================================
- Hits        38234    12492    -25742     
- Misses       4115    29824    +25709     
Impacted Files Coverage Δ
datalad/support/tests/test_locking.py 0.00% <0.00%> (-95.66%) ⬇️
datalad/support/locking.py 95.23% <88.88%> (-2.27%) ⬇️
datalad/tests/test_api.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/support/digests.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_base.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_config.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/ui/tests/__init__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test__main__.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/tests/test_strings.py 0.00% <0.00%> (-100.00%) ⬇️
datalad/ui/tests/test_base.py 0.00% <0.00%> (-100.00%) ⬇️
... and 248 more

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 a1a2106...cc180b8. Read the comment docs.

@bpoldrack
Copy link
Member

Since it seems like fasteners will not only regain the attribute but actually remove the release from pypi, we might want to give them 24h or so, before reacting with a quick fix, I think (much longer doesn't really work with entire CI broken).

@yarikoptic
Copy link
Member Author

0.16.2 is released, tested locally that our "locking" tests now back to normal. But I think we should still proceed with this PR, even if without major rush ;-) As mentioned in harlowja/fasteners#71 (comment) it is pretty much not advised to rely on it, and our code can be easily RFed as done here to not. It is only the tests are kinda troublesome and I forgot all this locking kitchen to possibly find a better way to fix up the tests without changing the API.

@bpoldrack
Copy link
Member

I forgot all this locking kitchen to possibly find a better way to fix up the tests without changing the API.

Never really looked into it, but it seems to me, that the solution is not to not return aquired, but get that information in a different way (not using this particular .acquired). Quick look is suggesting to me that something like .lockfile is not None will do as a replacement.

@yarikoptic
Copy link
Member Author

Never really looked into it, but it seems to me, that the solution is not to not return aquired, but get that information in a different way (not using this particular .acquired). Quick look is suggesting to me that something like .lockfile is not None will do as a replacement.

IIRC I had considered that option too but for some reason decided to just return... Looking at the code:

        self._do_open()
        watch = _utils.StopWatch(duration=timeout)
        r = _utils.Retry(delay, max_delay,
                         sleep_func=self.sleep_func, watch=watch)
        with watch:
            gotten = r(self._try_acquire, blocking, watch)
        if not gotten:
            self.acquired = False
            return False
        else:
            self.acquired = True
            self.logger.log(_utils.BLATHER,
                            "Acquired file lock `%s` after waiting %0.3fs [%s"
                            " attempts were required]", self.path,
                            watch.elapsed(), r.attempts)
            return True

where .lockfile is set/opened in _do_open, and then tried to get locked in _try_acquire - it seems that it is not guaranteed that .lockfile would be None if acquired = False. That is why I thought that the cleanest way is to somehow bubble up the acquired bool.

@bpoldrack
Copy link
Member

Yeah. So - just store the return value of acquire() in lock_if_check_fails?

@yarikoptic
Copy link
Member Author

Yeah. So - just store the return value of acquire() in lock_if_check_fails?

well, I already store it in a variable now in that function: https://github.com/datalad/datalad/pull/5718/files#diff-c4170b6a46f8d90d2bdcb206966fe9f16e8a431395245d39434edb681ab03ca6R94 . The question is how to channel back to the test on either lock was acquired or not (if not blocking). Having slept on it, I will just document that the _return_acquired is "private" and should not be used by 3rd party code, and be done with this one. pushed now - I still think we should go with this RFing to avoid future breakage-by-fasteners

@yarikoptic yarikoptic added merge-if-ok OP considers this work done, and requests review/merge semver-patch Increment the patch version when merged labels Jun 9, 2021
@bpoldrack bpoldrack merged commit 0891b8a into datalad:maint Jun 10, 2021
@yarikoptic yarikoptic deleted the bf-fasteners branch October 8, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants