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)

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).

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.

Patch looks fine to me. Is it easily testable?

> 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?

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.

> 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.

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?) ?

> 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.

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

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

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

You change caused test_socketserver to hang. I attempted a fix, but I'm not sure if it's completely correct.

> 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...

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

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. :)

> It's a bit of a rite of passage for new developers to
> break the buildbots. :)

How long is this rite?

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.

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

this would still be nice to have fixed ... any progress?

> 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.

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.