-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag) #66563
Comments
Linux 3.15 and newer support a vastly superior API for file locking, in which locks are owned by open file descriptions instead of by processes. This is how everyone seems to expect POSIX locks to work, but now they can finally work that way. Please add some interface to these locks to fcntl.lockf. One option would be to use them by default and to fall back to standard POSIX locks if they're not available. I don't know whether this would break existing code. See http://man7.org/linux/man-pages/man2/fcntl.2.html for details. |
I'd like to use fd locks from python too. |
This implements the open_file_descriptor argument and fixes a bug with converting to int when off_t is 64-bit but long is 32-bit. |
Please extract the fix into a different patch. |
Strange, Rietveld (the tool linked with the [review] button) doesn't show the change on the Modules/fcntlmodule.c file. By the way, I suggest to not include the Modules/clinic/fcntlmodule.c.h change in the patch. |
Here is the OFD patch, I'll open another issue for the overflow bug. |
bpo-38602 added constants, but this issue also modify lockf() to add open_file_descriptor parameter to choose between F_SETLK/F_SETLKW and F_OFD_SETLK/F_OFD_SETLKW. |
One question: |
According to https://www.gnu.org/software/libc/manual/html_node/Open-File-Description-Locks.html It is important to distinguish between the open file description (an instance of an open file, usually created by a call to open) and an open file descriptor, which is a numeric value that refers to the open file description. The locks described here are associated with the open file description and not the open file descriptor |
Failure on x86-64 High Sierra 3.8: Please fix tests, or revert your change: ====================================================================== Traceback (most recent call last):
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/test/test_fcntl.py", line 149, in test_lockf_exclusive
p.start()
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/process.py", line 121, in start
self._popen = self._Popen(self)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/context.py", line 224, in _Popen
return _default_context.get_context().Process._Popen(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/context.py", line 283, in _Popen
return Popen(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/popen_spawn_posix.py", line 32, in __init__
super().__init__(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/popen_fork.py", line 19, in __init__
self._launch(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/popen_spawn_posix.py", line 47, in _launch
reduction.dump(process_obj, fp)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/reduction.py", line 60, in dump
ForkingPickler(file, protocol).dump(obj)
AttributeError: Can't pickle local object 'TestFcntl.test_lockf_exclusive.<locals>.try_lockf_on_other_process' ====================================================================== Traceback (most recent call last):
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/test/test_fcntl.py", line 163, in test_lockf_share
p.start()
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/process.py", line 121, in start
self._popen = self._Popen(self)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/context.py", line 224, in _Popen
return _default_context.get_context().Process._Popen(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/context.py", line 283, in _Popen
return Popen(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/popen_spawn_posix.py", line 32, in __init__
super().__init__(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/popen_fork.py", line 19, in __init__
self._launch(process_obj)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/popen_spawn_posix.py", line 47, in _launch
reduction.dump(process_obj, fp)
File "/Users/buildbot/buildarea/3.8.billenstein-sierra/build/Lib/multiprocessing/reduction.py", line 60, in dump
ForkingPickler(file, protocol).dump(obj)
AttributeError: Can't pickle local object 'TestFcntl.test_lockf_share.<locals>.try_lockf_on_other_process' |
Victor, Thanks for letting me know. |
The AIX bot's are also in the red zone with PR17010. This was examined earlier See: https://bugs.python.org/issue35633#msg333662 In short, the recommendation by Victor was to skip the test: quote:
I would prefer to simply skip the lockf() test rather than ignoring PermissionError for flock() and lockf() on all platforms. And so, Lib/test/eintrdata/eintr_tester.py now has:
Thanks for your understanding. |
@Michael.Felt Thanks for the suggestion. cc @vstinner |
Dear Core developers Although I updated the unit test for this issue if the reverting is a better way. Please let me know. Thanks always |
Could PR17010 be reverted? For 10 days now several bots, AIX and x86-64 High Sierra - afaik, are failing the tests. re: https://bugs.python.org/issue22367#msg356614 - while that may address High Sierra, it does not address AIX. See message https://bugs.python.org/issue35633#msg333662 - wherein Victor states his preference to ignore the test (for AIX). A additional change to your next PR could be to also ignore AIX for this test. AIX returns a different error, "Permission Error", iirc. |
ignore my last comment - I missed your comment about skipping the test. My apologies. I'll be patient. Thanks for the update! |
This cause of failure PR 17010 was due to change of start methods as follows: So I updated the test through PR 17154 and I check that my local mac machine works well. (I did not check it on my local mac machine when I submitted PR 17010 since open description lock was Linux feature, this was my mistake) And thanks to the tip from Michael Felt, I update the test to skip on the AIX machine. So I expect that when PR 17154 is landed, everything will go well. ➜ cpython git:(bpo-22367-test-fix) ✗ ./python.exe Lib/test/test_fcntl.py -v ---------------------------------------------------------------------- Ran 10 tests in 0.246s OK (skipped=1) |
Bump. Serhiy, are you planning to follow up on this? |
FYI: I loaded the pr just now and tested on AIX. $ ./python -m test test_fcntl
0:00:00 Run tests sequentially
0:00:00 [1/1] test_fcntl == Tests result: SUCCESS == 1 test OK. Total duration: 767 ms $ git status
On branch pr_17154 Thanks |
Note: this is affecting the release of Python 3.9.0a1. I will be continuing with the release in 12 hours. If the failing macOS test is not fixed by then, alpha1 will ship in this state. However, I will be blocking alpha2 if this is still the case. Please prioritize fixing this. |
https://dev.azure.com/python/cpython/_build/results?buildId=54136&view=results Looks okay at this time. Thank you everyone who helps me. |
Thanks for the update to the PR FYI One AIX bot seems to be having support issues atm (and stays red), but the other one turned green again. 😄 |
p.s. the new PR also needs to be backported for the 3.8 bots. |
And the other AIX bot has been repaired, and is running green as well! :) |
It looks like the only thing left to do yet for this issue is to finish the review of and merge the PR that actually implements the new parameter. Removing the Deferred Blocker status and deselecting releases other than 3.9. |
@b8choi Please re-investigate this issue :) |
I'll take a look at this issue. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: