Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag) #66563

AndrewLutomirski · 2014-09-08T20:18:50Z

BPO	22367
Nosy	@pitrou, @vstinner, @ned-deily, @ambv, @serhiy-storchaka, @hvenev, @aixtools, @corona10, @miss-islington, @iforapsy
PRs	bpo-22367: Add test_lockf #17010 [3.8] bpo-22367: Add tests for fcntl.lockf(). (GH-17010) #17084 [3.7] bpo-22367: Add tests for fcntl.lockf(). (GH-17010) #17085 bpo-22367: Add open_file_descriptor parameter to fcntl.lockf() #17099 bpo-22367: Update test_fcntl.py for spawn process mode #17154 [3.8] bpo-22367: Update test_fcntl.py for spawn process mode (GH-17154) #17252 [3.7] bpo-22367: Update test_fcntl.py for spawn process mode (GH-17154) #17253
Files	0001-fcntl-support-F_OFD_.patch 0002-fcntl-Support-Linux-open-file-descriptor-locks.patch

^{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:

assignee = 'https://github.com/corona10'
closed_at = None
created_at = <Date 2014-09-08.20:18:49.508>
labels = ['type-feature', 'library', '3.9']
title = 'Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag)'
updated_at = <Date 2019-12-07.20:10:28.686>
user = 'https://bugs.python.org/AndrewLutomirski'

bugs.python.org fields:

activity = <Date 2019-12-07.20:10:28.686>
actor = 'ned.deily'
assignee = 'corona10'
closed = False
closed_date = None
closer = None
components = ['Library (Lib)']
creation = <Date 2014-09-08.20:18:49.508>
creator = 'Andrew.Lutomirski'
dependencies = []
files = ['42085', '42093']
hgrepos = []
issue_num = 22367
keywords = ['patch']
message_count = 34.0
messages = ['226610', '261295', '261310', '261353', '261354', '261365', '355512', '355522', '356212', '356295', '356296', '356328', '356329', '356607', '356614', '356675', '356678', '356681', '356815', '356816', '356837', '356849', '356852', '356913', '356938', '356952', '356954', '357251', '357253', '357255', '357264', '357286', '357287', '357984']
nosy_count = 11.0
nosy_names = ['pitrou', 'vstinner', 'ned.deily', 'lukasz.langa', 'serhiy.storchaka', 'Andrew.Lutomirski', 'h.venev', 'Michael.Felt', 'corona10', 'miss-islington', 'iforapsy']
pr_nums = ['17010', '17084', '17085', '17099', '17154', '17252', '17253']
priority = None
resolution = None
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue22367'
versions = ['Python 3.9']

AndrewLutomirski · 2014-09-08T20:18:49Z

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.

hvenev · 2016-03-07T11:08:12Z

I'd like to use fd locks from python too.

hvenev · 2016-03-07T17:55:03Z

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.

vstinner · 2016-03-08T12:18:28Z

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.

vstinner · 2016-03-08T12:19:47Z

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.

hvenev · 2016-03-08T15:43:40Z

Here is the OFD patch, I'll open another issue for the overflow bug.

corona10 · 2019-10-28T04:29:13Z

On bpo-38602, the constant related to open file descriptors will be added.
I will finalize this issue after bpo-38602 is closed.

vstinner · 2019-10-28T10:59:27Z

On bpo-38602, the constant related to open file descriptors will be added. I will finalize this issue after bpo-38602 is closed.

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.

serhiy-storchaka · 2019-11-07T20:31:45Z

New changeset befa032 by Serhiy Storchaka (Dong-hee Na) in branch 'master':
bpo-22367: Add tests for fcntl.lockf(). (GH-17010)
befa032

miss-islington · 2019-11-09T11:12:33Z

New changeset 85e4151 by Miss Islington (bot) in branch '3.8':
bpo-22367: Add tests for fcntl.lockf(). (GH-17010)
85e4151

miss-islington · 2019-11-09T11:12:40Z

New changeset 917dbe3 by Miss Islington (bot) in branch '3.7':
bpo-22367: Add tests for fcntl.lockf(). (GH-17010)
917dbe3

corona10 · 2019-11-10T15:30:21Z

One question:
Is there any reason to choose the name is open_file_descriptor not open_file_description?

corona10 · 2019-11-10T15:51:56Z

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

vstinner · 2019-11-14T14:00:21Z

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'

corona10 · 2019-11-14T16:22:27Z

@vstinner

Victor, Thanks for letting me know.
It was reproduced on my mac machine so I fix the test.
Please take a look.

aixtools · 2019-11-15T14:10:28Z

@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.

corona10 · 2019-11-15T14:39:51Z

@Michael.Felt

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

cc @vstinner

corona10 · 2019-11-15T15:03:00Z

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

aixtools · 2019-11-17T15:22:37Z

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.

aixtools · 2019-11-17T15:24:27Z

ignore my last comment - I missed your comment about skipping the test. My apologies.

I'll be patient.

Thanks for the update!

corona10 · 2019-11-18T02:13:58Z

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)

ned-deily · 2019-11-18T06:58:56Z

Bump. Serhiy, are you planning to follow up on this?

aixtools · 2019-11-18T08:44:18Z

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

ambv · 2019-11-18T21:40:50Z

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.

corona10 · 2019-11-19T01:04:24Z

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.

ambv · 2019-11-19T08:12:49Z

New changeset 9960230 by Łukasz Langa (Dong-hee Na) in branch 'master':
bpo-22367: Update test_fcntl.py for spawn process mode (bpo-17154)
9960230

corona10 · 2019-11-19T08:32:41Z

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.

aixtools · 2019-11-22T08:00:57Z

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. 😄

aixtools · 2019-11-22T08:05:46Z

p.s. the new PR also needs to be backported for the 3.8 bots.

corona10 · 2019-11-22T08:11:43Z

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.

aixtools · 2019-11-22T10:30:21Z

And the other AIX bot has been repaired, and is running green as well!

:)

vstinner · 2019-11-22T14:15:40Z

New changeset c3cd0de by Victor Stinner (Miss Islington (bot)) in branch '3.8':
bpo-22367: Update test_fcntl.py for spawn process mode (GH-17154) (GH-17252)
c3cd0de

vstinner · 2019-11-22T14:15:46Z

New changeset d4d7920 by Victor Stinner (Miss Islington (bot)) in branch '3.7':
bpo-22367: Update test_fcntl.py for spawn process mode (GH-17154) (GH-17253)
d4d7920

ned-deily · 2019-12-07T20:10:29Z

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.

corona10 · 2023-03-06T08:25:09Z

@b8choi Please re-investigate this issue :)

b8choi · 2023-03-06T08:26:24Z

I'll take a look at this issue.

AndrewLutomirski mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Sep 8, 2014

vstinner changed the title ~~Please add F_OFD_SETLK, etc support to fcntl.lockf~~ Add F_OFD_SETLK, etc support to fcntl.lockf Mar 8, 2016

vstinner changed the title ~~Add F_OFD_SETLK, etc support to fcntl.lockf~~ Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag) Mar 8, 2016

corona10 self-assigned this Oct 28, 2019

aixtools added 3.7 (EOL) end of life 3.8 only security fixes 3.9 only security fixes labels Nov 17, 2019

ned-deily added the deferred-blocker label Nov 18, 2019

ned-deily removed 3.7 (EOL) end of life 3.8 only security fixes deferred-blocker labels Dec 7, 2019

ezio-melotti transferred this issue from another repository Apr 10, 2022

corona10 added 3.12 bugs and security fixes and removed 3.9 only security fixes labels Mar 6, 2023

corona10 assigned b8choi Mar 6, 2023

erlend-aasland added 3.13 new features, bugs and security fixes and removed 3.12 bugs and security fixes labels Jan 5, 2024

Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag) #66563

Add open_file_descriptor parameter to fcntl.lockf() (use the new F_OFD_SETLK flag) #66563

Comments

AndrewLutomirski mannequin commented Sep 8, 2014

AndrewLutomirski mannequin commented Sep 8, 2014

hvenev mannequin commented Mar 7, 2016

hvenev mannequin commented Mar 7, 2016

vstinner commented Mar 8, 2016

vstinner commented Mar 8, 2016

hvenev mannequin commented Mar 8, 2016

corona10 commented Oct 28, 2019

vstinner commented Oct 28, 2019

serhiy-storchaka commented Nov 7, 2019

miss-islington commented Nov 9, 2019

miss-islington commented Nov 9, 2019

corona10 commented Nov 10, 2019

corona10 commented Nov 10, 2019

vstinner commented Nov 14, 2019

corona10 commented Nov 14, 2019

aixtools commented Nov 15, 2019

corona10 commented Nov 15, 2019

corona10 commented Nov 15, 2019

aixtools commented Nov 17, 2019

aixtools commented Nov 17, 2019

corona10 commented Nov 18, 2019

ned-deily commented Nov 18, 2019

aixtools commented Nov 18, 2019

ambv commented Nov 18, 2019

corona10 commented Nov 19, 2019

ambv commented Nov 19, 2019

corona10 commented Nov 19, 2019

aixtools commented Nov 22, 2019

aixtools commented Nov 22, 2019

corona10 commented Nov 22, 2019

aixtools commented Nov 22, 2019

vstinner commented Nov 22, 2019

vstinner commented Nov 22, 2019

ned-deily commented Dec 7, 2019

corona10 commented Mar 6, 2023

b8choi commented Mar 6, 2023