conch connection-sharing should be disabled by default, at least until it works a little

[glyph]

There are a bunch of issues with it right now:

• #3483
• #3497
• #716

In the short term it should just be disabled by default.

[glyph]

Highest priority because there's not much point to doing this if it doesn't get done before those other tickets.

[z3p]

(In [25083]) Branching to 'disable-connection-sharing-3498'

[z3p]

Ready for review.

[exarkun]

I guess there aren't any existing tests for a connection established with twisted.conch.client.connect.connect using a connection-sharing unix socket if the options don't specify how to try to set up a connection. If options explicitly ask for unix, that'll still work, since this branch only changes the default behavior, and all the tests for the unix socket explicitly ask for it, it seems. (Just summarizing because it wasn't immediately obvious why this change didn't break any tests).

When this feature is re-added, it'll need better test coverage, but that's kind of the idea of this ticket, so no one should be surprised by that. :)

The change does introduce a functional regression, though. If you connect to example.com with conch, then when you try to connect to that host again, the second attempt fails because both processes try to listen on the connection-sharing unix socket.

[z3p]

The "functional regression" is a known-issue when connecting twice: it's the first part of #3497. I'm not sure what that means for this branch, though.

[glyph]

Let me be more specific: by "disabled" I meant conch should neither listen nor attempt to connect using a shared UNIX socket. Both halves of it should be disabled; turning off one half of it seems to make the problems worse.

[z3p]

Connection sharing is now disabled by default, but can be (heaven forbid) enabled with commandline options. Now conch can connect to the same host twice.

[glyph]

Unfortunately, this causes several tests to fail, and some to hang. trial twisted.conch should rapidly show you some errors.

[z3p]

Ah; I forgot to update the "tests" which test the connection-sharing. I've updated them to use --cache to force it on. The test suite passes for me now.

[therve]

I have just one question:

-    useConnects = options.conns or ['unix', 'direct']
+    useConnects = options.conns or ['direct']

I don't understand how this change is related? Can you enlighten me?

[z3p]

The list at the end represents the default connection types that Conch uses to connect to a server. By removing 'unix' from that list, that makes direct connections the only connection type a plain conch <server> will try. A user can still specify -K unix,direct in order to use connection sharing.

[z3p]

No comments, so I'm putting this back up for review.

[exarkun]

After discussion with Paul on IRC about the possibilities here, it seems like the best plan is to completely remove the connection caching code now and then revisit the functionality later.

This is good because:

• it's easier to review a simple deletion than to review a change to this largely untested code
• it'll be easier to review straight additions later on than to review changes to existing code
• there are probably some problems with the --cache approach taken in the branch currently - like the fact that various parts of conch look at the presence of the --nocache option in order to determine if caching is in use or not (which is no longer correct with the addition of --cache)

[z3p]

Deleted the whole thing, including the old tests. I didn't add any new ones; I'm not even sure what they'd look like.

[exarkun]

The branch has some failures on Windows. Some of them look like they're due to the branch being a bit stale, but at least one looks real:

[ERROR]: twisted.conch.test.test_cftp

Traceback (most recent call last):
  File "c:\twistedbot\winxp32-py2.6-select\Twisted\twisted\trial\runner.py", line 557, in loadPackage
    module = modinfo.load()
  File "c:\twistedbot\winxp32-py2.6-select\Twisted\twisted\python\modules.py", line 380, in load
    return self.pathEntry.pythonPath.moduleLoader(self.name)
  File "c:\twistedbot\winxp32-py2.6-select\Twisted\twisted\python\reflect.py", line 456, in namedAny
    topLevelPackage = _importAndCheckStack(trialname)
  File "c:\twistedbot\winxp32-py2.6-select\Twisted\twisted\python\reflect.py", line 392, in _importAndCheckStack
    return __import__(importName)
  File "c:\twistedbot\winxp32-py2.6-select\Twisted\twisted\conch\test\test_cftp.py", line 640, in <module>
    TestOurServerUnixClient.skip = "don't run w/o spawnProcess or PyCrypto"
exceptions.NameError: name 'TestOurServerUnixClient' is not defined

Generally the changes look good. :) New tests typically aren't necessary when code is only being deleted. We need to make this change pretty visible in the next release's notes since it's an incompatible one - but I think the incompatible change is the right one to make, since this code had lots of bugs and is primarily of interest for performance reasons.

[z3p]

(In [26227]) Branching to 'disable-connection-sharing-3498-2'

[z3p]

I'm still getting an error on the branch, but it's an error that's in trunk too.

[glyph]

I guess I'm feeling more conservative now than the last time I did when I reviewed this:

1. --no-cache or -K will now generate tracebacks, whereas they would have worked before in (for example) a shell script.
2. The unix module should stick around and get deprecated like usual. Deleting it might break code that simply imported it by accident but didn't actually use it.

I really just want this branch to flip the default so that connecting twice to the same host works, not perform any surgery or remove any code; the equivalent of --nocache -K direct, all the time.

[exarkun]

I asked Paul to delete all of the code which implements this feature in an earlier review. If you and I disagree about that, then we probably need to talk about it directly, not push him back and forth in different directions.

[glyph]

Replying to exarkun:

I asked Paul to delete all of the code which implements this feature in an earlier review.

With that, and a successful buildbot run, in mind, I withdraw my objections.

I really should have been clearer from the beginning about what I wanted from this change. I thought it should only be a few lines. Please just merge this and get it over with :).

Paul, after you've merged, if you wouldn't mind poking the launchpad and expandrive people just to make sure that this didn't actually break anything, I'd appreciate it.

[exarkun]

The  build results look good (the gtk and Python 2.3 failures are unrelated to this branch). So I'm going to merge this.

[exarkun]

(In [26756]) Merge disable-connection-sharing-3498-2

Author: z3p Reviewer: exarkun, therve, glyph Fixes: #3498 Refs #3483, #3497, #716

Remove the Conch SSH connection sharing functionality. This feature is not well tested and has numerous subtle bugs which can interfer with the normal operation of the Conch SSH client.

The Conch command line arguments which supported configuring this functionality have been removed (supplying them is now an error) has has the module primarily responsible for the implementation.

It is expected that this feature will be re-introduced at some point, without the problems afflicting this implementation.