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

Change os.sendfile so its arguments are stable #59283

Open
larryhastings opened this issue Jun 15, 2012 · 11 comments
Open

Change os.sendfile so its arguments are stable #59283

larryhastings opened this issue Jun 15, 2012 · 11 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@larryhastings
Copy link
Contributor

BPO 15078
Nosy @loewis, @vstinner, @larryhastings, @giampaolo, @serhiy-storchaka

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/larryhastings'
closed_at = None
created_at = <Date 2012-06-15.10:53:38.116>
labels = ['type-bug']
title = 'Change os.sendfile so its arguments are stable'
updated_at = <Date 2012-06-23.23:08:19.144>
user = 'https://github.com/larryhastings'

bugs.python.org fields:

activity = <Date 2012-06-23.23:08:19.144>
actor = 'larry'
assignee = 'larry'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2012-06-15.10:53:38.116>
creator = 'larry'
dependencies = []
files = []
hgrepos = []
issue_num = 15078
keywords = []
message_count = 11.0
messages = ['162886', '162900', '162937', '162939', '162946', '163006', '163545', '163548', '163550', '163555', '163692']
nosy_count = 6.0
nosy_names = ['loewis', 'vstinner', 'larry', 'giampaolo.rodola', 'neologix', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = None
stage = None
status = 'open'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue15078'
versions = ['Python 3.3']

@larryhastings
Copy link
Contributor Author

As I keep saying on python-dev: I think that the argument list for a function should be stable. If you have a function where some abilities are only available on certain platforms, it's best to always accept default no-op parameters for those parameters, rather than adding and removing parameters based on what functionality is available.

os.sendfile() accepts either four or seven parameters, depending on the current platform. However, it's a new function in trunk and therefore has no installed base. So it's not too late to change it.

I propose to amend os.sendfile so it always accepts all seven arguments. Its signature would therefore be the following, on all platforms:

os.sendfile(out, in, offset, nbytes, headers=None, trailers=None, flags=0)

Passing in a non-None value for headers or trailers, or a nonzero value for flags, on a platform where the seven-argument form of sendfile is not available would raise NotImplementedError.

@larryhastings larryhastings self-assigned this Jun 15, 2012
@larryhastings larryhastings added the type-bug An unexpected behavior, bug, or error label Jun 15, 2012
@serhiy-storchaka
Copy link
Member

I believe, that instead of a integer flags will be better and more portable to use boolean parameters (diskio=True, wait=True, sync=False). All additional parameters should be keyword-only.

os.sendfile(out, in, offset, nbytes, *, headers=None, trailers=None, diskio=True, wait=True, sync=False)

@larryhastings
Copy link
Contributor Author

os.sendfile(out, in, offset, nbytes, *, headers=None, trailers=None, diskio=True, wait=True, sync=False)

I probably prefer this. If the original implementers are okay with it then I'd be happy to do it that way. But at the very least I want to get rid of the two function signatures.

@vstinner
Copy link
Member

As I keep saying on python-dev: I think that the argument list for a function should be stable.

There are some other functions with strange/variable prototype:
http://docs.python.org/dev/library/fcntl.html?highlight=ioctl#fcntl.ioctl
http://docs.python.org/dev/library/mmap.html?highlight=mmap.mmap#mmap.mmap
(and maybe others)

These Python functions are thin wrapper of the C function, that's why their prototype can be surprising.

@larryhastings
Copy link
Contributor Author

There are some other functions with strange/variable prototype:
http://docs.python.org/dev/library/fcntl.html?highlight=ioctl#fcntl.ioctl
http://docs.python.org/dev/library/mmap.html?highlight=mmap.mmap#mmap.mmap

It does not follow that this behavior is desirable, or that it should be emulated in new code. There's a lot of legacy code with unexpected API design that would not pass muster were it proposed today.

Consider this: what should the Signature (PEP-362) for fcntl.ioctl look like?

These Python functions are thin wrapper of the C function,
that's why their prototype can be surprising.

The "thin wrapper" os.sendfile is 130 lines of C. Furthermore I disagree with the idea that os does or should contain "thin wrappers". It strikes me as unbearably provincial thinking among UNIX developers. Anyone who thinks os contains "thin wrappers" should examine the Windows implementation of stat().

But at the heart of the matter, I see no benefit to exposing Python developers to the idiosyncrasies of poor C API design. I feel strongly that one way Python becomes "pythonic" is that it aims for the convenience of the programmer--not the language designer and not the implementer. The Python calling convention is far more flexible than the C calling convention. We should put it to good use here.

@giampaolo
Copy link
Contributor

I'm -0 about unifying the function signature and raise NotImplementedError where an arg is not supported.

I'm -1 about os.sendfile(..., diskio=True, wait=True, sync=False).

In both cases users willing to use these 3 args are already supposed to know what they're doing and that the code they're writing is not portable hence in practical terms I can't see a real gain in making a change.

@neologix
Copy link
Mannequin

neologix mannequin commented Jun 23, 2012

But at the heart of the matter, I see no benefit to exposing Python
developers to the idiosyncrasies of poor C API design. I feel strongly that
one way Python becomes "pythonic" is that it aims for the convenience of the
programmer--not the language designer and not the implementer. The Python
calling convention is far more flexible than the C calling convention. We
should put it to good use here.

I agree.

However, I think Martin is a proponent of the "thin wrapper" approach,
so it'd be nice to have his input on this.

I personally like the change, except for flags argument collapsing. Imagine what mmap's prototype would look like if we used list of optional arguments instead of a flag...

@serhiy-storchaka
Copy link
Member

I personally like the change, except for flags argument collapsing. Imagine what mmap's prototype would look like if we used list of optional arguments instead of a flag...

What's wrong with mmap? It uses list of optional arguments (flags,
prot, access) and not only one flags argument.

@loewis
Copy link
Mannequin

loewis mannequin commented Jun 23, 2012

I indeed think that the code is fine as it stands, and no change is needed, and that the proposed changes make matters worse.

The point of the thin wrappers approach is that you can read the manpage of your system, and immediately can trust that this is what the Python function will do. It is unfortunate that BSD and Linux have chosen to give the function the same name despite the signature differences, but there is no value in hiding this fact from the Python user.

The whole point of this function is performance and zero copy. Anybody using it will need to understand well what they are doing, and that their code is highly system-dependent. If you want cross-platform code, use shutil.copyfileobj.

I could agree to a higher-level function that tries to avoid system differences, but that function shouldn't be called sendfile. For example, the socket object could have a sendfd or sendstream method which would use the proper variant of sendfile if available, else uses a regular read/send loop.

I always found the name "sendfile" confusing, anyway, since it's not the file that is being sent, but the all (or some) of the contents of the file.

@neologix
Copy link
Mannequin

neologix mannequin commented Jun 23, 2012

What's wrong with mmap? It uses list of optional arguments (flags,
prot, access) and not only one flags argument.

Of course it does, as the mmap syscall(), since this arguments have nothing to do with one another.
I was refering to your proposal of splitting sendfile's flags argument, which is currently a bitmask, into distinct arguments (diskio=True, wait=True, sync=False).

If we did this for, let's say, mmap() flags, this would end up in a bazillion optional arguments, because there a re so many possible values for flags (MAP_SHARED, MAP_PRIVATE, MAP_ANONYMOUS, MAP_DENYWRITE...).
Bitmasks are a clear and compact way to pass optional arguments, and should be kept.

@larryhastings
Copy link
Contributor Author

It is usually folly to disagree with MvL, and yet I am about to do just that. As stated, I don't believe in the "thin wrappers" theory of os; where would a Windows programmer find the man page to learn how os.access works on their system?

I think os should provide helpful functionality to the programmer, specifically functions that are calling into the system API. But unlike MvL I would *much* rather see consistent functionality across systems than expose the Python programmer to every fiddling nook and cranny of the local API. I prefer "Python" programs to "Python-on-Linux" and "Python-on-Windows" programs.

Yet, as much as I'm willing to disagree with MvL, I'm not so sure of myself that I'll check this in against his wishes. If this idea wins more support in the future, we could still make my proposed change to the interface without breaking backwards-compatibility. So I volunteer to table the discussion at least for the 3.3 release.

TBH I don't see the benefit of the extra three parameters anyway. headers/trailers is redundant with os.writev, and the three flags are for shaving off cycles that are lost in the noise of the Python interpreter. If I ran the circus I'd remove the latter three parameters and have the same interface everywhere sendfile is available. But I suspect there's no way I can get away with *that* for the 3.3 beta, and once we hit feature freeze I won't be able to remove it. So I guess we're just going to be stuck with them forever. *sigh*

--

Let me controversially, hypothetically, propose: if we really want to make raw OS functions available to Python programmers, then so be it, but not in the os module. We could add an "osx" module, and a "linux" module, and a "freebsd" module, and a "win32" module (though I suppose win32all covers that). And we could ditch the whole "rename posix to os" shenanigans.

@ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
@iritkatriel iritkatriel added the extension-modules C modules in the Modules dir label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

5 participants