[MERGE] FTP test server for python2.[456]

[Vincent Ladeuil]

>>>>> "martin" == Martin Pool <mbp at canonical.com> writes:

    martin> Martin Pool has voted tweak.
    martin> Status is now: Conditionally approved
    martin> Comment:
    martin> +class _FTPServerFeature(tests.Feature):
    martin> +    """Some tests want an FTP Server, check if one is available.
    martin> +
    martin> +    Right now, the only way this is available is if 'medusa' is
    martin> installed.
    martin> +    http://www.amk.ca/python/code/medusa.html
    martin> +    """

    martin> That docstring seems inconsistent with the rest of your patch...

:-) I realized that after pressing the 'send' button. Fixed.

    martin> +class BZRConformingFTPHandler(ftpserver.FTPHandler):

    martin> Oddly inconsistent capitalization here.

Fixed (looks like brain emphasized the bzr by pressing shift :)

    martin> +class ftp_server(ftpserver.FTPServer):
    martin> +
    martin> +    def __init__(self, address, handler, authorizer):
    martin> +        ftpserver.FTPServer.__init__(self, address, handler)
    martin> +        self.authorizer = authorizer
    martin> +        # Worth backporting upstream ?
    martin> +        self.addr = self.socket.getsockname()
    martin> +
    martin> +

    martin> There's also FtpServer in the same file; it's just too many
    martin> things with the same name.

Inherited from medusa based design, depending on who is speaking
all are FTP servers, I'll fix that by renaming FTPServer to
FTPTestServer for the one seen from the tests.

I use the distinction between server and test server more
consistently in the bzr-local-test-server plugin, I can followup
with a patch renaming all of our servers (test or not) across the
test suite if you want.

    martin> +        """See bzrlib.transport.Server.tearDown."""
    martin> +        # Tell the server to stop, but also close the server
    martin> socket for tests
    martin> +        # that start the server but never initiate a
    martin> connection. Closing the
    martin> +        # socket should be done first though, to avoid further
    martin> connections.
    martin> +        self._ftp_server.close()
    martin> +        self._ftpd_running = False
    martin> +        self._ftpd_thread.join()
    martin> +

    martin> I wonder if this will tend to hang waiting for the
    martin> server thread, but I guess we can deal with it if it
    martin> happens.

This happened a lot with the HTTP servers for reasons fixed long
ago (wrong locking among others). And for a long time, any hang
in trying to join the server thread had always be caused by true
bugs in the HTTP server.

But I didn't encounter any problem while writing this patch (and
the corresponding mockup in bzr-local-test-server prior to that),
so I'm pretty sure no hang should happen here.

What is not evident here is that by setting _ftpd_running to
False, we trigger a server cleanup that will close all pending
client sockets, the server socket being closed just before ensure
that no more clients can connect in between.

As a side note, that cleanup method is missing for HTTP servers
and is needed to address the leaking threads (as well as a way to
collect the sockets and threads spawned for each connection).

    martin> Also it should be in NEWS.

Will do.

    martin> Aside from that it's good.

Thanks,

        Vincent