msg96482 - (view) |
Author: (lekma) * |
Date: 2009-12-16 11:00 |
It would be nice to have those.
See http://udrepper.livejournal.com/20407.html for background.
|
msg96484 - (view) |
Author: (lekma) * |
Date: 2009-12-16 12:31 |
First attempt, against trunk.
|
msg96504 - (view) |
Author: (lekma) * |
Date: 2009-12-17 07:24 |
better patch (mainly test refactoring).
still against trunk.
should I provide one against py3k?
|
msg96513 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-12-17 13:50 |
A couple of things:
- the test shouldn't be marked as Linux-specific since perhaps these
constants will be supported by other systems some day
- when importing fcntl, bear in mind that some systems don't have this
module (e.g. Windows), so you should catch and silence the ImportError
|
msg96551 - (view) |
Author: (lekma) * |
Date: 2009-12-18 07:52 |
> - when importing fcntl, bear in mind that some systems don't have this
> module (e.g. Windows), so you should catch and silence the ImportError
would something like the following be ok?:
try:
import fcntl
except ImportError:
fcntl = False
and then:
if hasattr(socket, "SOCK_CLOEXEC") and fcntl:
tests.append(CloexecLinuxConstantTest)
|
msg96559 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2009-12-18 11:14 |
It would be better to use test skipping: (eg: @unittest.SkipUnless
before the test class).
|
msg96598 - (view) |
Author: (lekma) * |
Date: 2009-12-19 09:58 |
> It would be better to use test skipping: (eg: @unittest.SkipUnless
> before the test class).
I didn't know about this feature, thanks for the tip.
Now I wonder if it would be better to do it this way:
@unittest.SkipUnless(hasattr(socket, "SOCK_CLOEXEC") and fcntl,
"SOCK_CLOEXEC not defined OR module fcntl not available")
or this way:
@unittest.SkipUnless(hasattr(socket, "SOCK_CLOEXEC"), "SOCK_CLOEXEC not
defined")
@unittest.SkipUnless(fcntl, "module fcntl not available")
the second option seems better to me (obvious reason why the test was
skipped), what do you guys think? (it doesn't really matter, I know, but
while we're here...)
|
msg96609 - (view) |
Author: R. David Murray (r.david.murray) * |
Date: 2009-12-19 16:09 |
I lean toward the second way, myself.
|
msg96666 - (view) |
Author: (lekma) * |
Date: 2009-12-20 09:18 |
this one addresses Antoines's comments (with the help of R. David Murray).
|
msg97636 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-01-12 17:53 |
The patch is basically fine. I'll add a "try .. finally" to the tests.
lekma, do you have a real name that we should add to the ACKS file?
|
msg97641 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-01-12 18:05 |
Looking at it again, there's the question of accept() behaviour. In the original code, it will call internal_setblocking() on the new fd if the listening socket is non-blocking. In the new code, if SOCK_NONBLOCK is defined it will not call any OS function to set the new fd in non-blocking mode.
Here is what the man page has to say:
On Linux, the new socket returned by accept() does not inherit file status flags such as
O_NONBLOCK and O_ASYNC from the listening socket. This behavior differs from the canonical
BSD sockets implementation. Portable programs should not rely on inheritance or non-inher‐
itance of file status flags and always explicitly set all required flags on the socket
returned from accept().
Linux has defined accept4() precisely for this purpose:
If flags is 0, then accept4() is the same as accept(). The following values can be bitwise
ORed in flags to obtain different behavior:
SOCK_NONBLOCK Set the O_NONBLOCK file status flag on the new open file description.
Using this flag saves extra calls to fcntl(2) to achieve the same result.
SOCK_CLOEXEC Set the close-on-exec (FD_CLOEXEC) flag on the new file descriptor. See
the description of the O_CLOEXEC flag in open(2) for reasons why this may
be useful.
|
msg97699 - (view) |
Author: (lekma) * |
Date: 2010-01-13 08:32 |
> lekma, do you have a real name that we should add to the ACKS file?
no, lekma is fine (that's a nick I carry since childhood)
> Looking at it again, there's the question of accept() behaviour. [...]
Sorry for not having seen that earlier and thanks for pointing it out to me.
Unfortunately, I don't think there is a portable solution. We could restrict the patch to linux 2.6.28 and above (there is already an awful lot of ifdefs in this file).
If you think that's the way to go, I can rework the patch, otherwise I think it's better to close the issue (maybe reopen when bsds catch up, if ever).
Ideas?
|
msg111182 - (view) |
Author: Mark Lawrence (BreamoreBoy) * |
Date: 2010-07-22 15:37 |
@Antoine could you respond to msg97699, thanks.
|
msg111183 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-07-22 15:46 |
> @Antoine could you respond to msg97699, thanks.
Well my comments and the patch itself are outdated now that 2.7 is in bugfix mode. The socket implementation in 3.2 is slightly different, which means the patch should be updated (it isn't necessarily difficult to do so), and the accept() issue should be examined again.
|
msg118457 - (view) |
Author: Vetoshkin Nikita (nvetoshkin) |
Date: 2010-10-12 18:53 |
Made an attempt to port lekma's patch to py3k-trunk. No (logical) changes needed.
Don't know about accept4() issue. As I saw in Qt sources, they ifdef'ed CLOEXEC by default on file descriptors. Don't think it's acceptable :) in this particular case. So, @Antoine, what do you say?
|
msg118463 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-10-12 19:29 |
The accept() issue is the following: the socket created by accept() (in Lib/socket.py) will formally inherit its parent's `type` attribute (including any SOCK_NONBLOCK and SOCK_CLOEXEC flags).
However, the underlying Linux socket is created by the accept() system call, which doesn't inherit flags as mentioned in the aforementioned man page. Therefore, the Python socket gives the wrong information about the socket's real flags.
This can be witnessed quickly:
>>> s = socket.socket(socket.AF_INET, socket.SOCK_STREAM | socket.SOCK_CLOEXEC)
>>> s.bind(("", 0))
>>> s.getsockname()
('0.0.0.0', 34634)
>>> s.listen(5)
>>> c, a = s.accept()
# Here, just start a "telnet" or "nc" session from another term
>>> import fcntl
>>> fcntl.fcntl(s, fcntl.F_GETFD)
1
>>> fcntl.fcntl(c, fcntl.F_GETFD)
0
>>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
0
>>> c.type & socket.SOCK_CLOEXEC
524288
The quick solution would be to mask out these flags when creating the Python socket in accept(). A better solution might be to inherit these flags by using the accept4() system call when possible (this is useful especially for SOCK_CLOEXEC, of course).
Apart from that, the patch looks ok, but it would be nice to test that at least the underlying socket is really in non-blocking mode, like is done in NonBlockingTCPTests.testSetBlocking.
|
msg118471 - (view) |
Author: Vetoshkin Nikita (nvetoshkin) |
Date: 2010-10-12 20:49 |
Thanks! I can see the problem now, but I think checking should be done like this:
>>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
0
>>> fcntl.fcntl(s, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
1
and with accept4() call I've got flag set:
>>> fcntl.fcntl(c, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
1
>>> fcntl.fcntl(s, fcntl.F_GETFD) & fcntl.FD_CLOEXEC
1
Don't know how to properly check if accept4 is available.
Second attempt - dropping flags from sock_type should be done on Python level in socket.py and I don't quite like idea to check if SOCK_CLOEXEC is in locals every time.
|
msg118473 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-10-12 21:03 |
You can check accept4() presence by adding it to the AC_CHECK_FUNCS macro in configure.in around line 2553, and add the corresponding HAVE_ACCEPT4 lines in pyconfig.h.in. Then run "autoconf" and you will have access to a HAVE_ACCEPT4 constant when accept4() exists.
By the way, you shouldn't use C++-style comments ("// ..."), because some C compilers refuse them.
|
msg118569 - (view) |
Author: Vetoshkin Nikita (nvetoshkin) |
Date: 2010-10-13 19:41 |
Another patch with:
- testInitBlocking method
- no c++ style comments
- a newly generated configure script (almost 1.5k lines diff)
- proper accept4 availability check
With this patch I've got
Traceback (most recent call last):
File "/home/nekto/workspace/py3k/Lib/test/test_socket.py", line 1564, in testInterruptedTimeout
foo = self.serv.accept()
socket.timeout: timed out
Can someone test if there's a real regression?
|
msg118581 - (view) |
Author: Vetoshkin Nikita (nvetoshkin) |
Date: 2010-10-13 20:49 |
Here's what strace on FAIL shows (print "in alarm_handler" added)
alarm(2) = 0
poll([{fd=3, events=POLLIN}], 1, 5000) = ? ERESTART_RESTARTBLOCK (To be restarted)
--- SIGALRM (Alarm clock) @ 0 (0) ---
rt_sigreturn(0xffffffff) = -1 EINTR (Interrupted system call)
accept4(3, 0x7fffa94c4780, [16], 0) = -1 EAGAIN (Resource temporarily unavailable)
poll([{fd=3, events=POLLIN}], 1, 2999) = 0 (Timeout)
accept4(3, 0x7fffa94c4780, [16], 0) = -1 EAGAIN (Resource temporarily unavailable)
write(1, "in alarm_handler\n", 17) = 17
alarm(0) = 0
Here's on OK:
alarm(2) = 0
poll([{fd=3, events=POLLIN}], 1, 5000) = ? ERESTART_RESTARTBLOCK (To be restarted)
--- SIGALRM (Alarm clock) @ 0 (0) ---
rt_sigreturn(0xffffffff) = -1 EINTR (Interrupted system call)
write(1, "in alarm_handler\n", 17) = 17
alarm(0)
For some reason does another trip through BEGIN_SELECT_LOOP() macro
|
msg118583 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-10-13 21:04 |
> For some reason does another trip through BEGIN_SELECT_LOOP() macro
Indeed:
if (!timeout)
+#ifdef HAVE_ACCEPT4
+ /* inherit socket flags and use accept4 call */
+ flags = s->sock_type & (SOCK_CLOEXEC | SOCK_NONBLOCK);
+ newfd = accept4(s->sock_fd, SAS2SA(&addrbuf), &addrlen, flags);
+#else
There's a missing curly brace after "if (!timeout)", so accept4() is always called.
|
msg118584 - (view) |
Author: Vetoshkin Nikita (nvetoshkin) |
Date: 2010-10-13 21:07 |
@Antoine already found that myself, patched and tested :) thanks!
|
msg118668 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-10-14 15:22 |
Patch committed in r85480, thanks!
|
msg146502 - (view) |
Author: Vetoshkin Nikita (nvetoshkin) |
Date: 2011-10-27 16:43 |
Started implementing accept4() socket method and stuck on socket object's timeout attribute. What value should we assign to sock->sock_timeout if SOCK_NONBLOCK was specified in accept4() call? And in socket.py should we check as in original accept:
if getdefaulttimeout() is None and self.gettimeout():
sock.setblocking(True)
|
msg146503 - (view) |
Author: Vetoshkin Nikita (nvetoshkin) |
Date: 2011-10-27 16:45 |
Sorry, wrong ticket. Right one is 10115
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:55 | admin | set | github: 51772 |
2011-10-27 16:45:04 | nvetoshkin | set | messages:
+ msg146503 |
2011-10-27 16:43:56 | nvetoshkin | set | messages:
+ msg146502 |
2010-10-14 15:22:38 | pitrou | set | status: open -> closed resolution: fixed messages:
+ msg118668
stage: needs patch -> resolved |
2010-10-13 21:07:38 | nvetoshkin | set | files:
+ issue7523_py3k_accept4_2.diff
messages:
+ msg118584 |
2010-10-13 21:04:12 | pitrou | set | messages:
+ msg118583 |
2010-10-13 20:49:51 | nvetoshkin | set | messages:
+ msg118581 |
2010-10-13 19:41:40 | nvetoshkin | set | files:
+ issue7523_py3k_accept4_1.diff
messages:
+ msg118569 |
2010-10-13 14:12:20 | giampaolo.rodola | set | nosy:
+ giampaolo.rodola
|
2010-10-12 21:03:36 | pitrou | set | messages:
+ msg118473 |
2010-10-12 20:49:13 | nvetoshkin | set | files:
+ issue7523_py3k_accept4.diff
messages:
+ msg118471 |
2010-10-12 19:29:47 | pitrou | set | messages:
+ msg118463 |
2010-10-12 18:53:27 | nvetoshkin | set | files:
+ issue7523_py3k.diff nosy:
+ nvetoshkin messages:
+ msg118457
|
2010-07-22 15:46:41 | pitrou | set | nosy:
+ exarkun messages:
+ msg111183
components:
- IO stage: patch review -> needs patch |
2010-07-22 15:37:16 | BreamoreBoy | set | nosy:
+ BreamoreBoy
messages:
+ msg111182 versions:
- Python 2.7 |
2010-01-13 08:32:44 | lekma | set | messages:
+ msg97699 |
2010-01-12 18:05:51 | pitrou | set | messages:
+ msg97641 |
2010-01-12 17:53:05 | pitrou | set | messages:
+ msg97636 |
2009-12-20 09:18:33 | lekma | set | files:
+ Issue7523_3.diff
messages:
+ msg96666 |
2009-12-19 16:09:05 | r.david.murray | set | messages:
+ msg96609 |
2009-12-19 09:58:45 | lekma | set | messages:
+ msg96598 |
2009-12-18 11:14:49 | r.david.murray | set | priority: normal
nosy:
+ r.david.murray messages:
+ msg96559
stage: patch review |
2009-12-18 07:53:00 | lekma | set | messages:
+ msg96551 |
2009-12-17 13:50:46 | pitrou | set | nosy:
+ pitrou messages:
+ msg96513
|
2009-12-17 07:24:52 | lekma | set | files:
+ Issue7523_2.diff
messages:
+ msg96504 components:
+ IO |
2009-12-16 12:31:33 | lekma | set | files:
+ Issue7523.diff keywords:
+ patch messages:
+ msg96484
|
2009-12-16 11:00:36 | lekma | create | |