This issue tracker has been migrated to GitHub ,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2009年04月07日 00:30 by ryan003, last changed 2022年04月11日 14:56 by admin.
| Files | ||||
|---|---|---|---|---|
| File name | Uploaded | Description | Edit | |
| unnamed | ryan003, 2011年05月24日 22:58 | |||
| Messages (22) | |||
|---|---|---|---|
| msg85680 - (view) | Author: Donghyun Kim (ryan003) | Date: 2009年04月07日 00:30 | |
During implement simple forking TCP server, I got the hang-up child process binding listen socket which caused parent(listen/accept) restarting failed. (port in use) Because child process does something with connected socket, there's no need to bind listen socket in child process. (finish_request() calls RequestHandlerClass with connected socket(=request)) Simply add self.socket.close() in the beginning of forked child process. SocketServer.ForkingMixIn : def process_request(self, request, client_address): """Fork a new subprocess to process the request.""" self.collect_children() pid = os.fork() if pid: # Parent process if self.active_children is None: self.active_children = [] self.active_children.append(pid) self.close_request(request) return else: # Child process. # This must never return, hence os._exit()! self.socket.close() # close parent's listen socket in child try: self.finish_request(request, client_address) os._exit(0) except: try: self.handle_error(request, client_address) finally: os._exit(1) |
|||
| msg136430 - (view) | Author: Charles-François Natali (neologix) * (Python committer) | Date: 2011年05月21日 11:11 | |
Thanks for reporting this, the current behaviour is clearly wrong. The child process doesn't need to - and shouldn't - inherit the server socket. The custom idiom when writting such code is to close the new socket (well, in TCP) in the parent process, and close the server socket in the child process. That's what the attached patch does. Since I really doubt anyone using socketserver ever used the server's socket from the handler, this shouldn't be a problem for existing code (and the server's socket was never documented to be usable from the request handler). |
|||
| msg136528 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2011年05月22日 14:04 | |
I don't know if it's related, but SimpleXMLRPCServer in Python 2.7 uses fcntl(self.fileno(), fcntl.F_SETFD, flags): class SimpleXMLRPCServer(SocketServer.TCPServer, SimpleXMLRPCDispatcher): ... def __init__(self, ...): ... # [Bug #1222790] If possible, set close-on-exec flag; if a # method spawns a subprocess, the subprocess shouldn't have # the listening socket open. if fcntl is not None and hasattr(fcntl, 'FD_CLOEXEC'): flags = fcntl.fcntl(self.fileno(), fcntl.F_GETFD) flags |= fcntl.FD_CLOEXEC fcntl.fcntl(self.fileno(), fcntl.F_SETFD, flags) => see also issue #1222790. |
|||
| msg136538 - (view) | Author: Antoine Pitrou (pitrou) * (Python committer) | Date: 2011年05月22日 15:23 | |
Patch looks fine to me. Is it easily testable? |
|||
| msg136569 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2011年05月22日 20:15 | |
> Patch looks fine to me. Is it easily testable? test_subprocess has some tests checking "cloexec": test_pipe_cloexec() and test_pipe_cloexec_real_tools(). You may reuse some code from these tests? |
|||
| msg136611 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2011年05月23日 11:11 | |
Oh, Linux 2.6.27+ has a SOCK_CLOEXEC option: we may use it (but it should be done in another issue). See also #12105. |
|||
| msg136620 - (view) | Author: Charles-François Natali (neologix) * (Python committer) | Date: 2011年05月23日 12:14 | |
> Oh, Linux 2.6.27+ has a SOCK_CLOEXEC option: It's not exactly the same thing. We want to close the socket right after fork, not wait until exec (in the OP case there was no exec). > Patch looks fine to me. Is it easily testable? Easily, no. |
|||
| msg136683 - (view) | Author: Charles-François Natali (neologix) * (Python committer) | Date: 2011年05月23日 17:07 | |
Antoine, do you think we can commit this as-is (i.e. without specific test)? If yes, to what branches (I'm not really sure of what kind of change is allowed for each branch, is there a document somewhere detailing the official policy?) ? |
|||
| msg136701 - (view) | Author: Antoine Pitrou (pitrou) * (Python committer) | Date: 2011年05月23日 22:04 | |
> Antoine, do you think we can commit this as-is (i.e. without specific > test)? Yes. > If yes, to what branches (I'm not really sure of what kind of change is > allowed for each branch, is there a document somewhere detailing the > official policy?) ? I think 2.7, 3.1, 3.2 and default. |
|||
| msg136762 - (view) | Author: Roundup Robot (python-dev) (Python triager) | Date: 2011年05月24日 16:23 | |
New changeset f13c06b777a7 by Charles-François Natali in branch '3.1': Issue #5715: In socketserver, close the server socket in the child process. http://hg.python.org/cpython/rev/f13c06b777a7 |
|||
| msg136763 - (view) | Author: Roundup Robot (python-dev) (Python triager) | Date: 2011年05月24日 16:27 | |
New changeset ccd59ba8145e by Charles-François Natali in branch '3.2': Issue #5715: In socketserver, close the server socket in the child process. http://hg.python.org/cpython/rev/ccd59ba8145e |
|||
| msg136765 - (view) | Author: Roundup Robot (python-dev) (Python triager) | Date: 2011年05月24日 16:30 | |
New changeset 0e56d79fa2ab by Charles-François Natali in branch 'default': Issue #5715: In socketserver, close the server socket in the child process. http://hg.python.org/cpython/rev/0e56d79fa2ab |
|||
| msg136773 - (view) | Author: Benjamin Peterson (benjamin.peterson) * (Python committer) | Date: 2011年05月24日 17:25 | |
You change caused test_socketserver to hang. I attempted a fix, but I'm not sure if it's completely correct. |
|||
| msg136774 - (view) | Author: Charles-François Natali (neologix) * (Python committer) | Date: 2011年05月24日 17:44 | |
> You change caused test_socketserver to hang. I attempted a fix, but I'm not sure if it's completely correct. I'm a morron. I don't know how I could miss this: closing the server socket is perfectly fine in TCP, since a new one is returned by accept(). But in UDP, it's definitely wrong, since it's used by the handler. I don't know however how I missed this, since I remember having run test_socketserver... The best fix is simply to revert the patch. I'm really sorry about this... |
|||
| msg136775 - (view) | Author: Roundup Robot (python-dev) (Python triager) | Date: 2011年05月24日 17:51 | |
New changeset 3e1709213532 by Benjamin Peterson in branch '3.1': backout 8b384de4e780, so a proper fix can be considered (#5715) http://hg.python.org/cpython/rev/3e1709213532 |
|||
| msg136776 - (view) | Author: Benjamin Peterson (benjamin.peterson) * (Python committer) | Date: 2011年05月24日 17:51 | |
2011年5月24日 Charles-François Natali <report@bugs.python.org>: > > Charles-François Natali <neologix@free.fr> added the comment: > >> You change caused test_socketserver to hang. I attempted a fix, but I'm not sure if it's completely correct. > > I'm a morron. > I don't know how I could miss this: closing the server socket is perfectly fine in TCP, since a new one is returned by accept(). But in UDP, it's definitely wrong, since it's used by the handler. > I don't know however how I missed this, since I remember having run test_socketserver... > > The best fix is simply to revert the patch. Done. > I'm really sorry about this... Don't worry. It's a bit of a rite of passage for new developers to break the buildbots. :) |
|||
| msg136778 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2011年05月24日 18:59 | |
> It's a bit of a rite of passage for new developers to > break the buildbots. :) How long is this rite? |
|||
| msg136781 - (view) | Author: Benjamin Peterson (benjamin.peterson) * (Python committer) | Date: 2011年05月24日 19:04 | |
2011年5月24日 STINNER Victor <report@bugs.python.org>: > > STINNER Victor <victor.stinner@haypocalc.com> added the comment: > >> It's a bit of a rite of passage for new developers to >> break the buildbots. :) > > How long is this rite? The approximate number of times you do it each year is e^(-x) + C where C >= 13. |
|||
| msg136804 - (view) | Author: Donghyun Kim (ryan003) | Date: 2011年05月24日 22:58 | |
On May 24, 2011, at 12:44 PM, Charles-François Natali wrote: > I don't know how I could miss this: closing the server socket is perfectly fine in TCP, since a new one is returned by accept(). But in UDP, it's definitely wrong, since it's used by the handler. > I don't know however how I missed this, since I remember having run test_socketserver... It's been a long time since the issue submitted, anyway, I was cursed to look at only TCP too :-) I agree that ForkingUDPServer should be supported in SocketServer.py. (Although users should take care of socket locking for concurrent accesses) How about using BaseServer(TCPServer).server_close() instead of self.socket.close() in the patch? As UDPServer has no server_close() method overridden, unlike ForkingTCPServer, ForkingUDPServer seems to have no actual "server" in design. So, I think we could say that - closing TCP listen socket in child process = "server_close()" in child process - nothing to do on UDP socket in child process = "server_close() but nothing will be done in the method" (b/c BaseServer.server_close() does nothing) What do you think? ----- Donghyun Kim http://www.uryan.net |
|||
| msg201837 - (view) | Author: Derek Wilson (underrun) | Date: 2013年10月31日 18:01 | |
this would still be nice to have fixed ... any progress? |
|||
| msg201852 - (view) | Author: STINNER Victor (vstinner) * (Python committer) | Date: 2013年10月31日 22:00 | |
> this would still be nice to have fixed ... any progress? Python 3.4 behaves a little bit better: all files and sockets are non-inheritable by default. It does not fix the issue if you fork without exec. To fix this issue, we need a patch. Nobody proposed something to fix the issue. |
|||
| msg260531 - (view) | Author: Martin Panter (martin.panter) * (Python committer) | Date: 2016年02月19日 22:15 | |
Replying to Donghyun’s last message, I understand UDPServer is derived from the TCPServer, so it would inherit TCPServer.server_close(), and therefore would close the one and only UDP socket. I think you may have to add a new prepare_child() method, or add a special case somewhere for forked TCP (or stream) servers. |
|||
| History | |||
|---|---|---|---|
| Date | User | Action | Args |
| 2022年04月11日 14:56:47 | admin | set | github: 49965 |
| 2016年02月19日 22:15:11 | martin.panter | set | nosy:
+ martin.panter messages: + msg260531 stage: patch review -> needs patch |
| 2013年10月31日 22:00:17 | vstinner | set | messages: + msg201852 |
| 2013年10月31日 18:01:13 | underrun | set | nosy:
+ underrun messages: + msg201837 versions: + Python 3.4, Python 3.5 |
| 2011年05月24日 22:58:14 | ryan003 | set | files:
+ unnamed messages: + msg136804 |
| 2011年05月24日 19:04:27 | benjamin.peterson | set | messages: + msg136781 |
| 2011年05月24日 18:59:00 | vstinner | set | messages: + msg136778 |
| 2011年05月24日 17:51:44 | benjamin.peterson | set | messages: + msg136776 |
| 2011年05月24日 17:51:25 | python-dev | set | messages: + msg136775 |
| 2011年05月24日 17:47:31 | neologix | set | files: - ss_fork_close.diff |
| 2011年05月24日 17:44:32 | neologix | set | messages: + msg136774 |
| 2011年05月24日 17:25:58 | benjamin.peterson | set | nosy:
+ benjamin.peterson messages: + msg136773 |
| 2011年05月24日 16:30:26 | python-dev | set | messages: + msg136765 |
| 2011年05月24日 16:27:58 | python-dev | set | messages: + msg136763 |
| 2011年05月24日 16:23:33 | python-dev | set | nosy:
+ python-dev messages: + msg136762 |
| 2011年05月23日 22:04:06 | pitrou | set | messages: + msg136701 |
| 2011年05月23日 17:07:44 | neologix | set | messages: + msg136683 |
| 2011年05月23日 12:14:21 | neologix | set | messages: + msg136620 |
| 2011年05月23日 11:11:07 | vstinner | set | messages: + msg136611 |
| 2011年05月22日 20:15:14 | vstinner | set | messages: + msg136569 |
| 2011年05月22日 15:23:41 | pitrou | set | messages:
+ msg136538 versions: + Python 3.3 |
| 2011年05月22日 14:04:29 | vstinner | set | messages: + msg136528 |
| 2011年05月22日 11:17:09 | vstinner | set | nosy:
+ pitrou |
| 2011年05月21日 11:11:43 | neologix | set | files:
+ ss_fork_close.diff nosy: + vstinner, neologix messages: + msg136430 keywords: + patch, needs review stage: test needed -> patch review |
| 2010年07月10日 16:12:04 | BreamoreBoy | set | stage: test needed versions: + Python 3.1, Python 2.7, Python 3.2, - Python 2.6, Python 2.4, Python 3.0 |
| 2009年04月14日 02:21:39 | victorpoluceno | set | nosy:
+ victorpoluceno |
| 2009年04月07日 00:30:48 | ryan003 | create | |