Code review comment for lp:~maxb/bzr/2.2-close-ssh-subprocess-socket

The code looks reasonable. I wonder if to be pedantically correct we should also be attempting to close the other side of the socketpair too, in case the subprocess doesn't close it for us? That would mean SubprocessVendor._connect should subproc_sock into SSHSubprocessConnection so it can pass it along to your improved _close_ssh_proc function with the other half. This may actually explain some test suite problems — I think this may actually be an fd leak. So please extend your patch to do this.

That brings me to my other suggestion: I think we should strive to write at least one direct unit test for this code. It's now had multiple bugs, and I think it's proven that the indirect test coverage so far isn't enough. Off the top of my head a test that constructs a SSHSubprocessConnection directly may be a reasonable way to do it, but perhaps instantiating a custom SubprocessVendor would be better? So if you can think of a way to write some direct tests for the fd/process management in ssh.py, please do. I won't block this patch for lack of tests as it's a clear improvement, but I'd love to see some :)

Finally, add a NEWS entry! :)