10308: Make SSHSession clean up both client and session #1696
Conversation
If an environment variable passing request and a subsystem request were both sent on the same channel, then SSHSession only cleaned up the ISession adapter when receiving EOF or closing the channel, and did not call loseConnection on the client transport which is the only reasonable way for a subsystem to be notified when a connection is closed. SSHSession now cleans up both the client transport and the ISession adapter if both are set.
There was a problem hiding this comment.
Looks good. Many thanks for the PR.
I can't think of any other backward compatible way to fix this.
For the long term, I hope we can get rid of self.client and only use the self.session to handle all kind of operations exec/shell/subsystem/env .
But maybe we don't need SSHSessionGen2 and we can deprecate SSHSession.client and add SSHSessionProcessProtocol.setSubsystem or something like that.
| @@ -180,16 +180,16 @@ def extReceived(self, dataType, data): | |||
| log.warn("Weird extended data: {dataType}", dataType=dataType) | |||
|
|
|||
| def eofReceived(self): | |||
| if self.client: | |||
There was a problem hiding this comment.
If it works for you. I think that is ok.
For me, the SSH session and subsystem experience is a bit of a mess ... both for server side and client side usage.
I have also hit this issue and did some fixed for my fork without knowing what is the right way to do it...so I have not sent a PR.
Happy to see this PR.
Basically, I found that having self.session and self.client and self.conn was confusing.
And I think that self.client is kind of a self.subsystem thing.
So I did some refactoring and removed the usage of self.session so that I only have self.client and self.conn
also... I have self.client.eofReceived() so that the client will know what happen and can take the appropriate actions... for example triggering a connection close.
And self.client is in fact a session ... but I found that better to not name it session and have a session in a session.
and to not have to use both self.client and self.session we can have something like
self.session.openSubsystem(client)I am aware that this might not be backward compatible.
.... so, in summary.
I am not happy with the current design of SSHSession ...but I don't know how we can untangle this in an easy way without breaking backward compatibility.
In general I am not happy with the current logic.
If we have a self.client, do something on the self.conn
I would prefer something like
If we have self.client, let the client know that eof was received.
Then the client can do what it wants.
But, I think that if this works for we, we should be fine.
For the long term, I think the only way out of this is to have a SSHSessionGen2 in which we don't have both session and client.
There was a problem hiding this comment.
Yeah, arranging for backward compatibility here is utterly miserable. We ran into this pretty hard when adding environment variable passing support as well.
self.client isn't just a subsystem thing - it's also used for communicating with shells or commands. But yes, I agree that this is all a mess. A further complication that I'm not sure you've considered is that environment variable passing requires creating an ISession adapter (self.session) before we're in a position to create an SSHSessionProcessProtocol (self.client), and the resulting ISession adapter must be kept around since otherwise the environment variable request would have no effect.
One approach that might work in a backward-compatible way would be to leave SSHSession.client and SSHSession.session as-is (unfortunate though that layout is), but move the session handling in SSHSession.eofReceived and SSHSession.closed into SSHSessionProcessProtocol; that way at least the protocol controls all the cleanup. However, for that to work properly we'd need to create an unconnected SSHSessionProcessProtocol a bit earlier in order to handle the case where an environment variable request has been sent but a shell/exec/subsystem request hasn't yet been sent, since currently self.session is set in that case but self.client isn't.
I think that could be done without a total backward-incompatible rewrite. What do you think? (But it feels like something that should be a separate PR.)
There was a problem hiding this comment.
A further complication that I'm not sure you've considered is that environment variable passing requires creating an ISession adapter (self.session) before we're in a position to create an SSHSessionProcessProtocol (self.client), and the resulting ISession adapter must be kept around since otherwise the environment variable request would have no effect.
Yes. I remember reviewing this. Thanks again for that fix :)
I think that could be done without a total backward-incompatible rewrite. What do you think? (But it feels like something that should be a separate PR.)
yes. A separate review.
I am happy to review such a PR :)
I will not have time to work on that refactoring anytime soon :(
|
|
||
| def closed(self): | ||
| if self.client and self.client.transport: | ||
| self.client.transport.loseConnection() |
There was a problem hiding this comment.
If it works for you, this is fine.
For my fork, I think that I found out that by calling self.client.transport.loseConnection() will trigger any close notifications on the client itself.
So here I am triggering something like self.client.closed() to also let the client know that it's over.
But i don't remember all the details...I remember that it was a bit of a mess :(
Here, in my fork I only have self.session.closed() ... but the session is linked to the client and the session will close the transport and notify the client.
There was a problem hiding this comment.
Yeah, the problem is that the session adapter can't really gain a link to the client protocol without significant backward-compatibility difficulties, since the session adapter is entirely provided by the application; so I think we do need these as separate concepts, although as mentioned above maybe the client protocol could gain a link to the session adapter and then it would be able to deal with notifying the session adapter.
Notifying the actual SSH client of the closure is dealt with by the self.conn.sendClose(self) call in SSHSession.eofReceived, which sends MSG_CHANNEL_CLOSE on the wire.
There was a problem hiding this comment.
Ah... sorry. I wanted to say. Notify the self.client subsystem... not the remote SSH peer.
Another reason to stop using self.client :)
Thanks for the merge!
When I tried to upgrade https://git.launchpad.net/turnip (the hosting backend for git.launchpad.net) to a backport of twisted#1696, I found that its SSH frontend tests failed with a series of extremely opaque errors which boiled down to git saying "fatal: the remote end hung up unexpectedly", and which on detailed comparison were because the "exit-status" channel request wasn't being sent after a "git upload-pack" subprocess exited. This turns out to be because I drew an incorrect parallelism between SSHSession.eofReceived and SSHSession.closed when fixing https://twistedmatrix.com/trac/ticket/10308: while it is indeed necessary to lose the client transport's connection in SSHSession.closed even if there's a session adapter as well, it's *not* necessary to send a close message in SSHSession.eofReceived if there's a session adapter. Firstly, the session adapter will typically already send a close message via SSHChannel.loseConnection once it's finished cleaning up (and would presumably have done so prior to the fix for https://twistedmatrix.com/trac/ticket/10308). Secondly, and more critically, the SSH_MSG_CHANNEL_CLOSE message sent by SSHConnection.sendClose is a request to terminate the channel, and it's supposed to be the last thing that the party sending it sends on that channel; SSHConnection refuses to send any other messages after sending a close message. As a result, any asynchronous cleanup work done by SSHSession.eofReceived, even a Deferred that fires on the next iteration of the reactor, will fail to send cleanup messages such as "exit-status" or "exit-signal", which the peer may interpret as abrupt termination; that is indeed what git does. We failed to notice this because the session tests use a StubConnection which behaves differently: it's happy to continue to "send" messages (i.e. record them in its various dicts of messages that would have been sent to a real peer by the code under test) even if the channel has been closed, and thus fails to be an accurate stub. Fixing the stub immediately exposes the failure in SessionInterfaceTests.test_requestExecWithData. Reverting part of my previous change means that SSHSession correctly sends "exit-status"/"exit-signal" messages again when a program it started in response to a "shell" or "exec" request exits. I've also checked that this doesn't cause https://twistedmatrix.com/trac/ticket/10308 to recur; only the change to SSHSession.closed was in fact needed there.
Scope and purpose
If an environment variable passing request and a subsystem request were
both sent on the same channel, then SSHSession only cleaned up the
ISession adapter when receiving EOF or closing the channel, and did not
call loseConnection on the client transport which is the only reasonable
way for a subsystem to be notified when a connection is closed.
SSHSession now cleans up both the client transport and the ISession
adapter if both are set.
I think this should be safe for non-subsystem sessions as well - I can't see a practical downside to doing more careful cleanup here.
Contributor Checklist:
tox -e lintto format my patch to meet the Twisted Coding Standard#character).reviewto the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.The first line is automatically generated by GitHub based on PR ID and branch name.
The other lines generated by GitHub should be replaced.