Code review comment for lp:~jameinel/bzr/2.3-bzr-connect-ssh

Well done !

A couple of tweaks and some comments.

8 +* Wait for the SSH server to actually finish, rather than just waiting for
9 + it to negotiate the key exchange. (John Arbash Meinel, #626876)
10 +

You commented on the abuse of the 'server' word but you use it here in a way that will
add to the confusion.

You're waiting for the *connection* to actually finish here and the way paramiko
presents it is misleading (using ssh_server as a variable name wasn't a good idea
in the first place either).

The SFTPServer uses TestingTCPServerInAThread and TestingSFTPServer which uses
TestingThreadingTCPServer.
TestingTCPServerInAThread is where the *server* is really shut down.
TestingThreadingTCPServer creates a new thread for each *connection*.

This connection should not start processing the request (the ssh command here)
before the key exchange. I.e TestingSFTPConnectionHandler.handle() which is
called after setup() should not start before the key exchange is done). The sleep
was enough... in some cases. But the join() *ensures* the synchronisation.

So:

28 server = tcs._server_interface(tcs)
29 + # This blocks until the key exchange has been done
30 ssh_server.start_server(None, server)

is misleading too, that's the join() that guarantees that the key exchange
has occurred and that it's safe to process the request. This comment applies
to the .join() indeed.

I think that one confusing point is that ssh presents an encrypted medium
(paramiko.Transport) from which one or several "connections" (in the TCP sense)
can happen (paramiko.Channel which can be used a socket).
We don't really care about the distinction in our case and view the Transport + Channel
as a single socket that appears unencrypted to us.

What this means is that we shouldn't touch the socket before the key exchange has occurred
and your fix ensures that.

Anyway, thank you for finding this nasty one, we may still want to avoid spawning
a subprocess here though and clean up some of the tearDown done here which I'm pretty
sure is now useless.