msg137054 - (view) |
Author: Charles-François Natali (neologix) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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) * |
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) |
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) * |
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) |
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) * |
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) * |
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) * |
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) * |
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) * |
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) * |
Date: 2011-05-29 21:03 |
The test now runs fine on the buildbots, closing.
|
msg137222 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
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 (vstinner) * |
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) * |
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 (vstinner) * |
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 (vstinner) * |
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) * |
Date: 2011-06-01 21:31 |
> requires_linux_version() should also be a decorator.
Patch attached.
|
msg137462 - (view) |
Author: STINNER Victor (vstinner) * |
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) * |
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) * |
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
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:17 | admin | set | github: 56405 |
2011-06-06 17:50:01 | python-dev | set | messages:
+ msg137761 |
2011-06-06 17:12:05 | pitrou | set | messages:
+ msg137755 |
2011-06-06 16:58:49 | neologix | set | files:
+ pipe2_arg.diff
messages:
+ msg137753 |
2011-06-03 11:00:08 | python-dev | set | messages:
+ msg137516 |
2011-06-01 22:05:46 | vstinner | set | messages:
+ msg137462 |
2011-06-01 21:31:55 | neologix | set | files:
+ support_linux_version.diff
messages:
+ msg137461 |
2011-06-01 18:31:20 | python-dev | set | messages:
+ msg137454 |
2011-06-01 11:25:42 | vstinner | set | messages:
+ msg137431 |
2011-05-31 20:27:03 | vstinner | set | messages:
+ msg137386 |
2011-05-31 17:28:13 | neologix | set | files:
+ pipe2_whatsnew.diff, support_linux_version.diff
messages:
+ msg137372 |
2011-05-30 19:40:05 | vstinner | set | messages:
+ msg137322 |
2011-05-30 17:23:47 | neologix | set | files:
+ support_linux_version.diff
messages:
+ msg137310 |
2011-05-30 04:58:40 | r.david.murray | set | messages:
+ msg137242 |
2011-05-29 22:00:12 | vstinner | set | messages:
+ msg137222 |
2011-05-29 21:03:03 | neologix | set | status: open -> closed resolution: fixed messages:
+ msg137220
stage: patch review -> resolved |
2011-05-29 18:53:00 | neologix | set | messages:
+ msg137213 |
2011-05-29 18:08:08 | python-dev | set | messages:
+ msg137210 |
2011-05-29 14:37:17 | python-dev | set | nosy:
+ python-dev messages:
+ msg137186
|
2011-05-28 20:05:22 | r.david.murray | set | nosy:
+ r.david.murray messages:
+ msg137150
|
2011-05-28 19:54:37 | gregory.p.smith | set | messages:
+ msg137149 |
2011-05-28 17:45:42 | neologix | set | files:
+ posix_pipe2.diff, support_pipe_max.diff
messages:
+ msg137140 |
2011-05-28 17:39:09 | neologix | set | files:
- support_pipe_max.diff |
2011-05-28 17:39:02 | neologix | set | files:
- posix_pipe2.diff |
2011-05-28 16:36:56 | gregory.p.smith | set | messages:
+ msg137139 |
2011-05-28 11:33:26 | rosslagerwall | set | messages:
+ msg137122 |
2011-05-28 10:44:35 | neologix | set | nosy:
+ gregory.p.smith
|
2011-05-28 10:42:42 | neologix | set | messages:
+ msg137118 |
2011-05-28 08:15:27 | rosslagerwall | set | nosy:
+ rosslagerwall messages:
+ msg137110
|
2011-05-27 16:34:42 | neologix | set | messages:
+ msg137081 |
2011-05-27 16:28:24 | neologix | set | files:
+ posix_pipe2.diff |
2011-05-27 16:27:50 | neologix | set | files:
- posix_pipe2.diff |
2011-05-27 16:02:24 | vstinner | set | messages:
+ msg137070 |
2011-05-27 15:52:43 | neologix | set | files:
+ posix_pipe2.diff |
2011-05-27 15:52:06 | neologix | set | files:
- posix_pipe2.diff |
2011-05-27 15:51:51 | neologix | set | files:
+ support_pipe_max.diff
messages:
+ msg137067 |
2011-05-27 14:34:36 | vstinner | set | messages:
+ msg137057 |
2011-05-27 14:11:01 | neologix | create | |