#6532 enhancement closed fixed (fixed)

twisted.internet.endpoints.SSHCommandClientEndpoint.connect with new connections should return a cancellable Deferred

Reported by: itamar Owned by: itamar
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/ssh-endpoint-cancellable-6532-2
(diff, github, buildbot, log)
Author: itamarst Launchpad Bug:

Description

If you are creating an SSH endpoint with a new connection each time, the Deferred returned for endpoint connection should be cancellable. It should call transport.abortConnnection on its TCP connection to abort the connection attempt if it's already connected and is in SSH protocol negotiation mode.

(This is a separate feature from cancellable reusable connections since those presumably don't want to close the shared TCP connection.)

Change History (14)

comment:1 Changed 16 months ago by itamarst

  • Author set to itamarst
  • Branch set to branches/ssh-endpoint-cancellable-6532

(In [38503]) Branching to 'ssh-endpoint-cancellable-6532'

comment:2 Changed 16 months ago by itamar

  • Owner set to itamar
  • Status changed from new to assigned

I believe the necessary work is:

  1. Extend the connection helpers with a destroyConnection method that does abortConnection on the protocol transport in the case of new connections helper. Presumably it does nothing for the existing connections helper.
  2. SSHCommandClientEndpoint._executeCommand should call destroyConnection rather than cleanupConnection on errors.
  3. Cancelling the factory.connectionReady Deferred should again call abortConnection on the transport.

comment:3 Changed 16 months ago by exarkun

Extend the connection helpers with a destroyConnection method that does abortConnection on the protocol transport in the case of new connections helper. Presumably it does nothing for the existing connections helper

Hmm, that's too bad - considering there's already cleanupConnection. Do we have to double all the interfaces now, so we have one API to ask for loseConnection and one API to ask for abortConnection? :/

comment:4 Changed 16 months ago by itamar

It's necessary in so far as cleanupConnection is also called in other code paths which want a clean cleanup, as it were. We could add an extra argument to cleanupConnection, instead of adding a new method, which should be fine as long as we merge this before next release since this is new code.

comment:5 Changed 16 months ago by exarkun

(In [38627]) Test cancel while waiting for exec without monkey-patching an implementation detail of the endpoint

refs #6532

comment:6 Changed 16 months ago by exarkun

(In [38628]) Merge private-conn-creator-interface-6541

Author: exarkun
Reviewer: jonathanj
Fixes: #6541

Change ISSHConnectionCreator to _ISSHConnectionCreator so as not to make it a
part of the public interface of twisted.conch.endpoints, pending some changes we
probably want to make to it (at least for cancellation - see #6532).

comment:7 Changed 16 months ago by itamarst

  • Branch changed from branches/ssh-endpoint-cancellable-6532 to branches/ssh-endpoint-cancellable-6532-2

(In [38630]) Branching to 'ssh-endpoint-cancellable-6532-2'

comment:8 Changed 16 months ago by itamar

  • Keywords review added
  • Owner changed from itamar to exarkun
  • Status changed from assigned to new

I've got a build running, haven't looked at it yet (actually two, you'll want to wait for the later one to finish).

comment:9 Changed 16 months ago by exarkun

  • Status changed from new to assigned

comment:10 Changed 16 months ago by exarkun

  • Keywords review removed
  • Owner changed from exarkun to itamar
  • Status changed from assigned to new

Thanks. Just a couple points, mostly about encapsulation:

  1. _CommandTransport now uses a lot of attributes from its creator which are not documented as part of _ISSHConnectionCreator. This code will really only work with a _NewConnectionHelper instance. It's true _CommandTransport was quite closely tied to _NewConnectionHelper before this change, but at least the documentation doesn't suggest otherwise. Now it has only the creator and uses a mix of documented and undocumented attributes from it. I'm not sure what resolution I'd like to see for this. Maybe the interface just needs to document all of these attributes? They're hardly well-considered though, it's just the collection of things I needed to implement the endpoint. There are clear problems with the collection as a while - for example, the issue of having keys separate from an agent. And stuff is missing from it, for example it cannot support keyboard-interactive authentication as is.
  2. The connection.transport.transport.abortConnection() necessary in the implementation of cleanupConnection makes me sad. This kind of deep object usage seems to be often necessary when working with conch. Is there something we could do to make this better? Should the conch SSH transport class implement abortConnection to help out here?

Sorry about the general vagueness of those points. Since this is all private stuff, it's not massively urgent to resolve it before this ticket can be closed. It would be nice to have at least an inkling of a plan about where we're going to go from here, though.

Please merge when you're satisfied these points have been addressed somehow, even if not necessarily by fixing all of the internals to be completely perfect.

comment:11 Changed 16 months ago by itamar

For the first item, how about just documenting that _CommandTransport only takes a _NewConnectionHelper, and then it's using expected public attributes?

comment:12 Changed 16 months ago by exarkun

For the first item, how about just documenting that _CommandTransport only takes a _NewConnectionHelper, and then it's using expected public attributes?

Sure, that works.

comment:13 Changed 16 months ago by itamar

OK, updated _CommandTransport.__init__ docstring, and added #6548 for point 2.

comment:14 Changed 16 months ago by itamarst

  • Resolution set to fixed
  • Status changed from new to closed

(In [38660]) Merge ssh-endpoint-cancellable-6532-2: Cancellable endpoints for new-connection SSH endpoint.

Fixes: #6532
Author: itamar, exarkun
Review: exarkun, itamar

If you use an SSH command endpoint that opens a new connection, you can cancel the connection attempt and SSH negotation by cancelling the connection Deferred.

Note: See TracTickets for help on using tickets.