classification
Title: add pipe2() to the os module
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: gregory.p.smith, haypo, neologix, pitrou, python-dev, r.david.murray, rosslagerwall
Priority: low Keywords: needs review, patch

Created on 2011-05-27 14:11 by neologix, last changed 2011-06-06 17:50 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
posix_pipe2.diff neologix, 2011-05-28 17:45 review
support_pipe_max.diff neologix, 2011-05-28 17:45 review
support_linux_version.diff neologix, 2011-05-30 17:23 review
pipe2_whatsnew.diff neologix, 2011-05-31 17:28 review
support_linux_version.diff neologix, 2011-05-31 17:28 review
support_linux_version.diff neologix, 2011-06-01 21:31 review
pipe2_arg.diff neologix, 2011-06-06 16:58 review
Messages (30)
msg137054 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-27 14:10
pipe2() makes it possible to create a pipe O_CLOEXEC or O_NONBLOCK atomically, which can be quite useful, especially in multi-threaded code. It would be nice to expose it in the os module.
Patch attached.
msg137057 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-05-27 14:34
+        self.assertRaises(TypeError, os.pipe2, (0, 0))

Do you want to call the function with two arguments or one tuple with 2 items? You may test both :-)

+        # try a write big enough to fill-up the pipe (64K on most kernels): this
+        # should perform a partial write, not block
+        os.write(w, b'x' * 100000)

This constant should be moved to test.support. BaseTestCase.test_communicate_pipe_buf() on subprocess uses:

        x, y = os.pipe()
        if mswindows:
            pipe_buf = 512
        else:
            pipe_buf = os.fpathconf(x, "PC_PIPE_BUF")

SignalsTest.check_interrupted_write() of test_io uses:

            # Fill the pipe enough that the write will be blocking.
            # It will be interrupted by the timer armed above.  Since the
            # other thread has read one byte, the low-level write will
            # return with a successful (partial) result rather than an EINTR.
            # The buffered IO layer must check for pending signal
            # handlers, which in this case will invoke alarm_interrupt().
            self.assertRaises(ZeroDivisionError,
                              wio.write, item * (1024 * 1024))

Instead of a constant, it may be function taking a pipe end as argument and returning the size of its buffer. Something like:

def pipe_buffer_size(fd):
    if hasattr(os, 'fpathconf'): # better than sys.platform == "win32"?
        pipe_buf = 512
    else:
        pipe_buf = os.fpathconf(fd, "PC_PIPE_BUF")

+        self.assertTrue((time.time() - start) < 1.0, "Test took too long for O_NONBLOCK.")

Hum, I'm not sure that it's revelant to test the time: if the call blocks, it will block forever. If the system is busy, the read+write may takes longer than 1 second.
msg137067 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-27 15:51
> Do you want to call the function with two arguments or one tuple with 2 items? You may test both :-)

The former. Fixed.

> Hum, I'm not sure that it's revelant to test the time

I didn't like it either, I just reused what's done in test_socket. Fixed.

> BaseTestCase.test_communicate_pipe_buf() on subprocess uses

Yeah, except that it doesn't work, POSIX's PIPE_BUF is the guaranteed limit for the write to be atomic, not the buffer size (furthermore the libc doesn't have any way to know the pipe's buffer size or max atomic write, it's just a defined constant).

$ cat /tmp/test_pipe.py 
import os

r, w = os.pipe()
max = os.fpathconf(w, 'PC_PIPE_BUF')
print(max)
os.write(w, 2 * max * b'x')
print('ok')

$ ./python /tmp/test_pipe.py 
4096
ok

> This constant should be moved to test.support.

Patch attached.
It uses a constant of 1MB, even on Windows. I hope it won't break there, you never know with Windows...
msg137070 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-05-27 16:02
+# A constant likely larger than the underlying OS pipe buffer size.
+# Windows limit seems to be around 512B, and most Unix kernels have a 64K pipe
+# buffer size: take 1MB to be sure.
+PIPE_MAX_SIZE = 1024 * 1024

Hum, I am not sure that the comment is correct. If I understood correctly the usage of this constant: write PIPE_MAX_SIZE into a blocking pipe must block because PIPE_MAX_SIZE is greater than the size of the pipe buffer. I don't know what happen if you write PIPE_MAX_SIZE into a nonblocking pipe: the beginning of the buffer is written and write() returns something lesser than PIPE_MAX_SIZE?

You may specify that if you should greater or equal to os.fpathconf(fd, "PC_PIPE_BUF").

-----------

                               'sys.stderr.write("xyz"*%d);'
-                              'sys.stdout.write(sys.stdin.read())' % pipe_buf],
+                              'sys.stdout.write(sys.stdin.read())' %
+                              support.PIPE_MAX_SIZE],

and

+        string_to_write = b"abc" * support.PIPE_MAX_SIZE

Here you use PIPE_MAX_SIZE*3, not PIPE_MAX_SIZE :-) You can use b'abc' * (support.PIPE_MAX_SIZE // 3), or b'a' * support.PIPE_MAX_SIZE.
msg137081 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-27 16:34
> +# A constant likely larger than the underlying OS pipe buffer size.
> +# Windows limit seems to be around 512B, and most Unix kernels have a 64K pipe
> +# buffer size: take 1MB to be sure.
> +PIPE_MAX_SIZE = 1024 * 1024
>
> Hum, I am not sure that the comment is correct. If I understood correctly the usage of this constant: write PIPE_MAX_SIZE into a blocking pipe must block because PIPE_MAX_SIZE is greater than the size of the pipe buffer.

Correct.

> I don't know what happen if you write PIPE_MAX_SIZE into a nonblocking pipe: the beginning of the buffer is written and write() returns something lesser than PIPE_MAX_SIZE?
>

Correct. Note that it could also fail with EAGAIN, that's why I added
an except OSError clause to the write.

> You may specify that if you should greater or equal to os.fpathconf(fd, "PC_PIPE_BUF").
>

I don't understand what you mean. It will be greater than PIPE_BUF,
which is around 4096 (I think 512 guaranteed by POSIX).

> Here you use PIPE_MAX_SIZE*3, not PIPE_MAX_SIZE :-) You can use b'abc' * (support.PIPE_MAX_SIZE // 3), or b'a' * support.PIPE_MAX_SIZE.
>

I know, but that's what the original code did, so I kept it.
msg137110 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-05-28 08:15
Out of interest, is there any reason that the configure check for pipe2 is a special case near the bottom of configure.in instead of with all the other function checks in the AC_CHECK_FUNCS[] section in the middle?
I know this patch didn't write the configure check but maybe it could be changed.

Also, the pure python implementation of subprocess for posix can now be updated to use pipe2 if it exists (previously on _posixsubprocess.c used it).
msg137118 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-28 10:42
> Out of interest, is there any reason that the configure check for pipe2 is a special case near the bottom of configure.in instead of with all the other function checks in the AC_CHECK_FUNCS[] section in the middle?

No clue. I'll fix that.

> Also, the pure python implementation of subprocess for posix can now be updated to use pipe2 if it exists (previously on _posixsubprocess.c used it).

I don't understand the last part :-)
What do you suggest?
Use os.pipe2 if available, otherwise use _posixsubprocess.c (because
it has the advantage of running with the GIL held) if available, and
use os.pipe + fcntl as last resort?

By the way, what's the reason for having a both a Python and a C implementation?
Are they some configurations where _posixsubprocess could not be available?
msg137122 - (view) Author: Ross Lagerwall (rosslagerwall) (Python committer) Date: 2011-05-28 11:33
>> Also, the pure python implementation of subprocess for posix can now >> be updated to use pipe2 if it exists (previously on _posixsubprocess.c
>> used it).

> I don't understand the last part :-)
> What do you suggest?

Perhaps, os.pipe2 can be used if available, then a modified version of _posixsubprocess.cloexec_pipe without the check for pipe2 could be used as a fallback.

I'm not sure why there exists both a python and c implementation.
msg137139 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-05-28 16:36
I just nuked the pure Python POSIX subprocess implementation in 70467:75ca834df824.  No need for both implementations.  _posixsubprocess is now the only option.
msg137140 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-28 17:45
> I just nuked the pure Python POSIX subprocess implementation

Nice.

I've updated both patches to address Victor's comments on test_io and
test_subprocess usage of PIPE_MAX_SIZE, and Ross' comment on pipe2's
configure tests.
I left subprocess use _posixsubprocess.cloexec_pipe, since it leads to
simpler code, especially now that the Python version has been removed.
msg137149 - (view) Author: Gregory P. Smith (gregory.p.smith) * (Python committer) Date: 2011-05-28 19:54
Include an appropriate Version Added annotation in the pipe2 documentation.

Otherwise the current patches look good to me.
msg137150 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-05-28 20:05
For the record (for people reading this ticket later), the removal of the python version of the unix subprocess code was discussed on IRC, and it was clarified there that the reason for removing it is not that it duplicates the C code.  We like having python implementations of C code where possible, but the key here is "where possible".  The python version of the Unix subprocess code cannot be made to work correctly, it is necessary to drop into C to get the semantics correct.
msg137186 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-29 14:37
New changeset 866d959dea8e by Charles-François Natali in branch 'default':
Issue #12196: Add PIPE_MAX_SIZE to test.support, constant larger than the
http://hg.python.org/cpython/rev/866d959dea8e
msg137210 - (view) Author: Roundup Robot (python-dev) Date: 2011-05-29 18:08
New changeset 293c398cd4cf by Charles-François Natali in branch 'default':
Issue #12196: Add pipe2() to the os module.
http://hg.python.org/cpython/rev/293c398cd4cf
msg137213 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-29 18:52
The Gentoo buildbot on which O_CLOEXEC test failed also chokes on test_pipe2:

[ 10/355] test_posix
test test_posix failed -- Traceback (most recent call last):
  File "/var/lib/buildslave/3.x.murray-gentoo-wide/build/Lib/test/test_posix.py", line 487, in test_pipe2
    r, w = os.pipe2()
OSError: [Errno 38] Function not implemented

If've added a test to skip this test on Linux kernels older than 2.6.27: like O_CLOEXEC, the problem is that the libc defines pipe2() while the kernel doesn't support it, so when syscall() is called it bails out with ENOSYS.
(This buildbot should really have its libc/kernel upgraded...)
msg137220 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-29 21:03
The test now runs fine on the buildbots, closing.
msg137222 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-05-29 22:00
> If've added a test to skip this test on Linux kernels older than 2.6.27:
> like O_CLOEXEC, the problem is that the libc defines pipe2() while the
> kernel doesn't support it, so when syscall() is called it bails out with
> ENOSYS. (This buildbot should really have its libc/kernel upgraded...)

You may add the issue number of your commit changelog, so a comment is 
generated directly in the issue.

support.linux_version() may be changed for requires_linux_version(2, 6, 27), 
but linux_version() is always used in tests to check the Linux version. 
requires_linux_version() would only raise a SkipTest if the OS is Linux and if 
the kernel is lesser than the specified version.

By the way, I like the new os.pipe2() function! You may want to document it in 
the "What's new in Python 3.3" doc (just mention the new function, the 
document will be rephrased later).
msg137242 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2011-05-30 04:58
FYI that buildbot is likely to get a kernel upgrade in the not too distant future.
msg137310 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-30 17:23
> support.linux_version() may be changed for requires_linux_version(2, 6, 27),
>
> but linux_version() is always used in tests to check the Linux version.
> requires_linux_version() would only raise a SkipTest if the OS is Linux and
> if
> the kernel is lesser than the specified version.
>

See the patch attached.

> By the way, I like the new os.pipe2() function! You may want to document it
> in
> the "What's new in Python 3.3" doc (just mention the new function, the
> document will be rephrased later).
>

Sure, where is this document?
msg137322 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-05-30 19:40
> See the patch attached.

I like your patch, it removes many duplicate code :-)

Some comments:
 - I don't like an if surrounding the whole function, I prefer "if not ...: 
return" to avoid the (useless) indentation. Well, it's my coding style, do as 
you want.
 - You should add docstrings. Something like "Raise a SkipTest if the OS is 
Linux and the kernel version if lesser than min_version. For example, 
support.requires_linux_version(2, 6, 28) raises a SkipTest if the version is 
lesser than 2.6.28."
 - You should move the "if version < ..." outside the try/except ValueError

+            # platform.release() is something like '2.6.33.7-desktop-2mnb'
+            version_string = platform.release().split('-')[0]

Dummy micro-optimization: .split('-', 1)[0] or .partition('-')[0] is maybe 
better than .split('-')[0].

> > By the way, I like the new os.pipe2() function! You may want to document
> > it in
> > the "What's new in Python 3.3" doc (just mention the new function, the
> > document will be rephrased later).
> 
> Sure, where is this document?

Doc/whatsnew/3.3.rst
msg137372 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-05-31 17:28
> I like your patch, it removes many duplicate code :-)
>
> Some comments:

Patch attached.

> Doc/whatsnew/3.3.rst

Patch attached.
msg137386 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-05-31 20:27
support_linux_version.diff: cool, it's even better than the previous patch. You 
can commit it, except if you are motived for a last change: display the write 
also version in the SkipTest message (as it is done actually).

pipe2_whatsnew.diff: I don't have any special to say, except that you didn't 
use ~ for :data:`os.O_NONBLOCK`. Go ahead.
msg137431 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-01 11:25
> support_linux_version.diff: cool, it's even better than the previous patch.
> You can commit it, except if you are motived for a last change: display
> the write also version in the SkipTest message (as it is done actually).

I just added a very similar function for Mac OS X which is a decorator. 
Example: @support.requires_mac_ver(10, 5).

requires_linux_version() should also be a decorator.
msg137454 - (view) Author: Roundup Robot (python-dev) Date: 2011-06-01 18:31
New changeset 4124d1f75b93 by Charles-François Natali in branch 'default':
Issue #12196: Add a note on os.pipe2() in the "Whats' new in Python 3.3"
http://hg.python.org/cpython/rev/4124d1f75b93
msg137461 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-01 21:31
> requires_linux_version() should also be a decorator.

Patch attached.
msg137462 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2011-06-01 22:05
Your last support_linux_version.diff patch looks good. If you are motivated, this new function can also be added to test.support of Python 3.2 (test_socket.py has the original linux_version() function).
msg137516 - (view) Author: Roundup Robot (python-dev) Date: 2011-06-03 11:00
New changeset 277bbe6cae53 by Charles-François Natali in branch 'default':
Issue #12196: Make test.support's requires_linux_version a decorator.
http://hg.python.org/cpython/rev/277bbe6cae53
msg137753 - (view) Author: Charles-François Natali (neologix) * (Python committer) Date: 2011-06-06 16:58
Hmm, thinking about it, I don't see any reason to make the flags argument optional.
Here's a patch changing that (also, pipe2 is now declared as METH_O instead of METH_VARARGS).
msg137755 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2011-06-06 17:12
> Hmm, thinking about it, I don't see any reason to make the flags argument optional.
> Here's a patch changing that (also, pipe2 is now declared as METH_O instead of METH_VARARGS).

Looks good to me.
msg137761 - (view) Author: Roundup Robot (python-dev) Date: 2011-06-06 17:50
New changeset ba975c7c33d3 by Charles-François Natali in branch 'default':
Issue #12196: Make os.pipe2() flags argument mandatory.
http://hg.python.org/cpython/rev/ba975c7c33d3
History
Date User Action Args
2011-06-06 17:50:01python-devsetmessages: + msg137761
2011-06-06 17:12:05pitrousetmessages: + msg137755
2011-06-06 16:58:49neologixsetfiles: + pipe2_arg.diff

messages: + msg137753
2011-06-03 11:00:08python-devsetmessages: + msg137516
2011-06-01 22:05:46hayposetmessages: + msg137462
2011-06-01 21:31:55neologixsetfiles: + support_linux_version.diff

messages: + msg137461
2011-06-01 18:31:20python-devsetmessages: + msg137454
2011-06-01 11:25:42hayposetmessages: + msg137431
2011-05-31 20:27:03hayposetmessages: + msg137386
2011-05-31 17:28:13neologixsetfiles: + pipe2_whatsnew.diff, support_linux_version.diff

messages: + msg137372
2011-05-30 19:40:05hayposetmessages: + msg137322
2011-05-30 17:23:47neologixsetfiles: + support_linux_version.diff

messages: + msg137310
2011-05-30 04:58:40r.david.murraysetmessages: + msg137242
2011-05-29 22:00:12hayposetmessages: + msg137222
2011-05-29 21:03:03neologixsetstatus: open -> closed
resolution: fixed
messages: + msg137220

stage: patch review -> resolved
2011-05-29 18:53:00neologixsetmessages: + msg137213
2011-05-29 18:08:08python-devsetmessages: + msg137210
2011-05-29 14:37:17python-devsetnosy: + python-dev
messages: + msg137186
2011-05-28 20:05:22r.david.murraysetnosy: + r.david.murray
messages: + msg137150
2011-05-28 19:54:37gregory.p.smithsetmessages: + msg137149
2011-05-28 17:45:42neologixsetfiles: + posix_pipe2.diff, support_pipe_max.diff

messages: + msg137140
2011-05-28 17:39:09neologixsetfiles: - support_pipe_max.diff
2011-05-28 17:39:02neologixsetfiles: - posix_pipe2.diff
2011-05-28 16:36:56gregory.p.smithsetmessages: + msg137139
2011-05-28 11:33:26rosslagerwallsetmessages: + msg137122
2011-05-28 10:44:35neologixsetnosy: + gregory.p.smith
2011-05-28 10:42:42neologixsetmessages: + msg137118
2011-05-28 08:15:27rosslagerwallsetnosy: + rosslagerwall
messages: + msg137110
2011-05-27 16:34:42neologixsetmessages: + msg137081
2011-05-27 16:28:24neologixsetfiles: + posix_pipe2.diff
2011-05-27 16:27:50neologixsetfiles: - posix_pipe2.diff
2011-05-27 16:02:24hayposetmessages: + msg137070
2011-05-27 15:52:43neologixsetfiles: + posix_pipe2.diff
2011-05-27 15:52:06neologixsetfiles: - posix_pipe2.diff
2011-05-27 15:51:51neologixsetfiles: + support_pipe_max.diff

messages: + msg137067
2011-05-27 14:34:36hayposetmessages: + msg137057
2011-05-27 14:11:01neologixcreate