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

Created on 2009-04-07 00:30 by ryan003, last changed 2016-02-19 22:15 by martin.panter.

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 (haypo) * (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 (haypo) * (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 (haypo) * (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) 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) 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) 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) 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 (haypo) * (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 (haypo) * (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
2016-02-19 22:15:11martin.pantersetnosy: + martin.panter

messages: + msg260531
stage: patch review -> needs patch
2013-10-31 22:00:17hayposetmessages: + 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:00hayposetmessages: + 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:07hayposetmessages: + msg136611
2011-05-22 20:15:14hayposetmessages: + msg136569
2011-05-22 15:23:41pitrousetmessages: + msg136538
versions: + Python 3.3
2011-05-22 14:04:29hayposetmessages: + msg136528
2011-05-22 11:17:09hayposetnosy: + pitrou
2011-05-21 11:11:43neologixsetfiles: + ss_fork_close.diff

nosy: + haypo, 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