homepage

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.

classification
Title: listen socket close in SocketServer.ForkingMixIn.process_request()
Type: behavior Stage: needs patch
Components: Library (Lib) Versions: Python 3.1, Python 3.2, Python 3.3, Python 3.4, Python 3.5, Python 2.7
process
Status: open Resolution:
Dependencies: Superseder:
Assigned To: Nosy List: benjamin.peterson, martin.panter, neologix, pitrou, python-dev, ryan003, underrun, victorpoluceno, vstinner
Priority: normal Keywords: needs review, patch

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:47adminsetgithub: 49965
2016年02月19日 22:15:11martin.pantersetnosy: + martin.panter

messages: + msg260531
stage: patch review -> needs patch
2013年10月31日 22:00:17vstinnersetmessages: + msg201852
2013年10月31日 18:01:13underrunsetnosy: + underrun

messages: + msg201837
versions: + Python 3.4, Python 3.5
2011年05月24日 22:58:14ryan003setfiles: + unnamed

messages: + msg136804
2011年05月24日 19:04:27benjamin.petersonsetmessages: + msg136781
2011年05月24日 18:59:00vstinnersetmessages: + msg136778
2011年05月24日 17:51:44benjamin.petersonsetmessages: + msg136776
2011年05月24日 17:51:25python-devsetmessages: + msg136775
2011年05月24日 17:47:31neologixsetfiles: - ss_fork_close.diff
2011年05月24日 17:44:32neologixsetmessages: + msg136774
2011年05月24日 17:25:58benjamin.petersonsetnosy: + benjamin.peterson
messages: + msg136773
2011年05月24日 16:30:26python-devsetmessages: + msg136765
2011年05月24日 16:27:58python-devsetmessages: + msg136763
2011年05月24日 16:23:33python-devsetnosy: + python-dev
messages: + msg136762
2011年05月23日 22:04:06pitrousetmessages: + msg136701
2011年05月23日 17:07:44neologixsetmessages: + msg136683
2011年05月23日 12:14:21neologixsetmessages: + msg136620
2011年05月23日 11:11:07vstinnersetmessages: + msg136611
2011年05月22日 20:15:14vstinnersetmessages: + msg136569
2011年05月22日 15:23:41pitrousetmessages: + msg136538
versions: + Python 3.3
2011年05月22日 14:04:29vstinnersetmessages: + msg136528
2011年05月22日 11:17:09vstinnersetnosy: + pitrou
2011年05月21日 11:11:43neologixsetfiles: + ss_fork_close.diff

nosy: + vstinner, neologix
messages: + msg136430

keywords: + patch, needs review
stage: test needed -> patch review
2010年07月10日 16:12:04BreamoreBoysetstage: 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:39victorpolucenosetnosy: + victorpoluceno
2009年04月07日 00:30:48ryan003create

AltStyle によって変換されたページ (->オリジナル) /