classification
Title: Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag)
Type: enhancement Stage: patch review
Components: Library (Lib) Versions: Python 3.9
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: corona10 Nosy List: Andrew.Lutomirski, Michael.Felt, corona10, h.venev, iforapsy, lukasz.langa, miss-islington, ned.deily, pitrou, serhiy.storchaka, vstinner
Priority: Keywords: patch

Created on 2014-09-08 20:18 by Andrew.Lutomirski, last changed 2019-12-07 20:10 by ned.deily.

Files
File name Uploaded Description Edit
0001-fcntl-support-F_OFD_.patch h.venev, 2016-03-07 17:55 review
0002-fcntl-Support-Linux-open-file-descriptor-locks.patch h.venev, 2016-03-08 15:43 review
Pull Requests
URL Status Linked Edit
PR 17010 merged corona10, 2019-10-31 06:50
PR 17084 merged miss-islington, 2019-11-07 20:32
PR 17085 merged miss-islington, 2019-11-07 20:32
PR 17099 open corona10, 2019-11-09 14:59
PR 17154 merged corona10, 2019-11-14 16:18
PR 17252 merged miss-islington, 2019-11-19 08:13
PR 17253 merged miss-islington, 2019-11-19 08:13
Messages (34)
msg226610 - (view) Author: Andrew Lutomirski (Andrew.Lutomirski) Date: 2014-09-08 20:18
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.
msg261295 - (view) Author: Hristo Venev (h.venev) * Date: 2016-03-07 11:08
I'd like to use fd locks from python too.
msg261310 - (view) Author: Hristo Venev (h.venev) * Date: 2016-03-07 17:55
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.
msg261353 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-08 12:18
> 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.
msg261354 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-03-08 12:19
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.
msg261365 - (view) Author: Hristo Venev (h.venev) * Date: 2016-03-08 15:43
Here is the OFD patch, I'll open another issue for the overflow bug.
msg355512 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-10-28 04:29
On issue 38602, the constant related to open file descriptors will be added.
I will finalize this issue after issue 38602 is closed.
msg355522 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-10-28 10:59
> On issue 38602, the constant related to open file descriptors will be added. I will finalize this issue after issue 38602 is closed.

issue 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.
msg356212 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2019-11-07 20:31
New changeset befa032d8869e0fab4732d910f3887642879d644 by Serhiy Storchaka (Dong-hee Na) in branch 'master':
bpo-22367: Add tests for fcntl.lockf(). (GH-17010)
https://github.com/python/cpython/commit/befa032d8869e0fab4732d910f3887642879d644
msg356295 - (view) Author: miss-islington (miss-islington) Date: 2019-11-09 11:12
New changeset 85e415108226cc5f3fdddd70196fc4c2a1d0f7f4 by Miss Islington (bot) in branch '3.8':
bpo-22367: Add tests for fcntl.lockf(). (GH-17010)
https://github.com/python/cpython/commit/85e415108226cc5f3fdddd70196fc4c2a1d0f7f4
msg356296 - (view) Author: miss-islington (miss-islington) Date: 2019-11-09 11:12
New changeset 917dbe350a762ed6d75b7d074f3fb87975ba717b by Miss Islington (bot) in branch '3.7':
bpo-22367: Add tests for fcntl.lockf(). (GH-17010)
https://github.com/python/cpython/commit/917dbe350a762ed6d75b7d074f3fb87975ba717b
msg356328 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-10 15:30
One question:
Is there any reason to choose the name is `open_file_descriptor` not `open_file_description`?
msg356329 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-10 15:51
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
msg356607 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-14 14:00
Failure on x86-64 High Sierra 3.8:
https://buildbot.python.org/all/#/builders/226/builds/519

Please fix tests, or revert your change:
https://pythondev.readthedocs.io/ci.html#revert-on-fail

======================================================================
ERROR: test_lockf_exclusive (test.test_fcntl.TestFcntl)
----------------------------------------------------------------------
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'

======================================================================
ERROR: test_lockf_share (test.test_fcntl.TestFcntl)
----------------------------------------------------------------------
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'
msg356614 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-14 16:22
@vstinner

Victor, Thanks for letting me know.
It was reproduced on my mac machine so I fix the test.
Please take a look.
msg356675 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-11-15 14:10
@corona10

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:
> On AIX the test for flock() passes, but the test for lockf() fails: (...)

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:

    @unittest.skipIf(platform.system() == "AIX", "AIX returns PermissionError")
    def test_lockf(self):
        self._lock(fcntl.lockf, "lockf")


Thanks for your understanding.
msg356678 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-15 14:39
@Michael.Felt

Thanks for the suggestion.
I 've updated the PR to skip the test on AIX.

cc @vstinner
msg356681 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-15 15:03
Dear Core developers

Although I updated the unit test for this issue if the reverting is a better way. Please let me know.
I am happy to follow the decision. :)

Thanks always
msg356815 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-11-17 15:22
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.
msg356816 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-11-17 15:24
ignore my last comment - I missed your comment about skipping the test. My apologies.

I'll be patient.

Thanks for the update!
msg356837 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-18 02:13
This cause of failure PR 17010 was due to change of start methods as follows:
https://docs.python.org/3/library/multiprocessing.html#contexts-and-start-methods
Changed in version 3.8: On macOS, the spawn start method is now the default. The fork start method should be considered unsafe as it can lead to crashes of the subprocess. See bpo-33725.


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
struct.pack:  b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00'
test_fcntl_64_bit (__main__.TestFcntl) ... skipped 'F_NOTIFY or DN_MULTISHOT unavailable'
test_fcntl_bad_file (__main__.TestFcntl) ... ok
test_fcntl_bad_file_overflow (__main__.TestFcntl) ... ok
test_fcntl_f_getpath (__main__.TestFcntl) ... ok
test_fcntl_file_descriptor (__main__.TestFcntl) ... Status from fcntl with O_NONBLOCK:  0
String from fcntl with F_SETLKW:  b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00'
ok
test_fcntl_fileno (__main__.TestFcntl) ... Status from fcntl with O_NONBLOCK:  0
String from fcntl with F_SETLKW:  b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00'
ok
test_flock (__main__.TestFcntl) ... ok
test_flock_overflow (__main__.TestFcntl) ... ok
test_lockf_exclusive (__main__.TestFcntl) ... struct.pack:  b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00'
ok
test_lockf_share (__main__.TestFcntl) ... struct.pack:  b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x03\x00\x00\x00'
ok

----------------------------------------------------------------------

Ran 10 tests in 0.246s

OK (skipped=1)
msg356849 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-11-18 06:58
Bump.  Serhiy, are you planning to follow up on this?
msg356852 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-11-18 08:44
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
Tests result: SUCCESS

$ git status
On branch pr_17154

Thanks
msg356913 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2019-11-18 21:40
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.
msg356938 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-19 01:04
Steve approved this PR 17154 but we need one more reviewer to review this PR.
I hope we can reflect PR 17154 before alpha1 is released.
msg356952 - (view) Author: Łukasz Langa (lukasz.langa) * (Python committer) Date: 2019-11-19 08:12
New changeset 9960230f76eb555d6dfbe8a324efed35610c85f9 by Łukasz Langa (Dong-hee Na) in branch 'master':
bpo-22367: Update test_fcntl.py for spawn process mode (#17154)
https://github.com/python/cpython/commit/9960230f76eb555d6dfbe8a324efed35610c85f9
msg356954 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-19 08:32
https://dev.azure.com/python/cpython/_build/results?buildId=54136&view=results

Looks okay at this time.
It was nerve-racking, I worried about my mistake affect publishing Python 3.9.0 alpha1.

Thank you everyone who helps me.
msg357251 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-11-22 08:00
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. :smile:
msg357253 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-11-22 08:05
p.s. the new PR also needs to be backported for the 3.8 bots.
msg357255 - (view) Author: Dong-hee Na (corona10) * (Python triager) Date: 2019-11-22 08:11
> p.s. the new PR also needs to be backported for the 3.8 bots.

PR 17252 and PR 17253 are backporting patches.
And they are ready to be merged :)

Thank you for the following up on this issue.
msg357264 - (view) Author: Michael Felt (Michael.Felt) * Date: 2019-11-22 10:30
And the other AIX bot has been repaired, and is running green as well!

:)
msg357286 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-22 14:15
New changeset c3cd0de9ec7ab54186cebef5b2edfd098f7ae387 by Victor Stinner (Miss Islington (bot)) in branch '3.8':
bpo-22367: Update test_fcntl.py for spawn process mode (GH-17154) (GH-17252)
https://github.com/python/cpython/commit/c3cd0de9ec7ab54186cebef5b2edfd098f7ae387
msg357287 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2019-11-22 14:15
New changeset d4d79209e69d52ea4c047545c6c60c3ba75f15f4 by Victor Stinner (Miss Islington (bot)) in branch '3.7':
bpo-22367: Update test_fcntl.py for spawn process mode (GH-17154) (GH-17253)
https://github.com/python/cpython/commit/d4d79209e69d52ea4c047545c6c60c3ba75f15f4
msg357984 - (view) Author: Ned Deily (ned.deily) * (Python committer) Date: 2019-12-07 20:10
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.
History
Date User Action Args
2020-01-15 21:37:57vstinnerlinkissue36746 superseder
2019-12-07 20:10:28ned.deilysetpriority: deferred blocker ->

messages: + msg357984
versions: - Python 3.5, Python 3.6, Python 3.7, Python 3.8
2019-11-22 14:15:46vstinnersetmessages: + msg357287
2019-11-22 14:15:40vstinnersetmessages: + msg357286
2019-11-22 10:30:21Michael.Feltsetmessages: + msg357264
2019-11-22 08:11:43corona10setmessages: + msg357255
2019-11-22 08:05:46Michael.Feltsetmessages: + msg357253
2019-11-22 08:00:57Michael.Feltsetmessages: + msg357251
2019-11-19 08:32:41corona10setmessages: + msg356954
2019-11-19 08:13:22miss-islingtonsetpull_requests: + pull_request16749
2019-11-19 08:13:10miss-islingtonsetpull_requests: + pull_request16748
2019-11-19 08:12:49lukasz.langasetmessages: + msg356952
2019-11-19 01:04:24corona10setmessages: + msg356938
2019-11-18 21:40:50lukasz.langasetnosy: + lukasz.langa
messages: + msg356913
2019-11-18 08:44:17Michael.Feltsetmessages: + msg356852
2019-11-18 06:58:56ned.deilysetpriority: normal -> deferred blocker
nosy: + ned.deily
messages: + msg356849

2019-11-18 06:57:11ned.deilylinkissue38790 superseder
2019-11-18 02:13:58corona10setmessages: + msg356837
2019-11-17 15:24:27Michael.Feltsetmessages: + msg356816
2019-11-17 15:22:37Michael.Feltsetmessages: + msg356815
versions: + Python 3.7, Python 3.8, Python 3.9
2019-11-15 15:03:00corona10setmessages: + msg356681
2019-11-15 14:39:50corona10setmessages: + msg356678
2019-11-15 14:10:28Michael.Feltsetnosy: + Michael.Felt
messages: + msg356675
2019-11-14 18:27:48neologixsetnosy: - neologix
2019-11-14 16:22:27corona10setmessages: + msg356614
2019-11-14 16:18:27corona10setpull_requests: + pull_request16664
2019-11-14 14:00:21vstinnersetmessages: + msg356607
2019-11-10 15:51:55corona10setmessages: + msg356329
2019-11-10 15:30:20corona10setmessages: + msg356328
2019-11-09 14:59:21corona10setpull_requests: + pull_request16607
2019-11-09 11:12:39miss-islingtonsetmessages: + msg356296
2019-11-09 11:12:32miss-islingtonsetnosy: + miss-islington
messages: + msg356295
2019-11-07 20:32:35miss-islingtonsetpull_requests: + pull_request16595
2019-11-07 20:32:27miss-islingtonsetpull_requests: + pull_request16594
2019-11-07 20:31:45serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg356212
2019-10-31 06:50:50corona10setstage: needs patch -> patch review
pull_requests: + pull_request16534
2019-10-28 10:59:26vstinnersetmessages: + msg355522
2019-10-28 04:29:13corona10setassignee: corona10

messages: + msg355512
nosy: + corona10
2019-07-29 18:33:58nanjekyejoannahsetnosy: - nanjekyejoannah
2019-07-19 18:58:27nanjekyejoannahsetnosy: + nanjekyejoannah
2019-07-18 00:53:39iforapsysetnosy: + iforapsy
2016-03-08 15:43:40h.venevsetfiles: + 0002-fcntl-Support-Linux-open-file-descriptor-locks.patch

messages: + msg261365
versions: + Python 3.6
2016-03-08 12:20:42vstinnersetnosy: + pitrou
2016-03-08 12:20:23vstinnersettitle: Add F_OFD_SETLK, etc support to fcntl.lockf -> Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag)
2016-03-08 12:19:47vstinnersetmessages: + msg261354
title: Please add F_OFD_SETLK, etc support to fcntl.lockf -> Add F_OFD_SETLK, etc support to fcntl.lockf
2016-03-08 12:18:27vstinnersetmessages: + msg261353
2016-03-07 17:55:02h.venevsetfiles: + 0001-fcntl-support-F_OFD_.patch
keywords: + patch
messages: + msg261310
2016-03-07 13:25:09vstinnersetnosy: + vstinner
2016-03-07 11:08:12h.venevsetnosy: + h.venev
messages: + msg261295
2014-11-18 13:31:03pitrousetnosy: + neologix
2014-11-18 09:48:27serhiy.storchakasetstage: needs patch
versions: - Python 2.7
2014-09-08 20:18:49Andrew.Lutomirskicreate