Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 359873

Summary:	[ssh] Thread leak for sshConnectorService
Product:	[Tools] Target Management	Reporter:	Simon Bernard <sbernard>
Component:	RSE	Assignee:	dsdp.tm.rse-inbox <tm.rse-inbox>
Status:	NEW ---	QA Contact:	Martin Oberhuber <mober.at+eclipse>
Severity:	normal
Priority:	P3	CC:	contact
Version:	3.2.1
Target Milestone:	---
Hardware:	PC
OS:	Windows XP
Whiteboard:

Description Simon Bernard

2011-10-04 12:40:11 EDT

Problem :
===========
  In some cases, the thread which manages the ssh session is not closed before a new one is created. This could involved some thread leak.

How to reproduce :
====================
  /!\ it's a bit complex slant.tif /!\
  I found this potential problem with this use case :
  - a systemtype based on ssh connector service.
  - the scp file subsystem (but any other subsystem based on ssh should do the trick)
  - a subsystem based on sshConnectorService with a custom isConnected method. This method returns true if we are connected to the ssh service and another tcp connection is already opened.

  When you click on connect in the UI, the SystemConnectAllSubSystemsAction is run. We try to connect to all the subsystems. 
 - for the first one we are not connected yet, so we open the ssh connection.
 - for the second, we should be connected (but we are not because the tcp connection failed), and so we try to reconnect the ssh connection 
 (see SubSystem#connect(org.eclipse.core.runtime.IProgressMonitor, boolean))

 In this case, we create a second session, while the first one has not been closed...
 (see SshConnectorService.internalConnect(IProgressMonitor))
  

Solution : 
==============
  Generaly a good practice is to close the current session before to create a new one in a "connect" method. In this case we are sure to never have phantom sessions.


I could try to work on a patch if you are agree this is a problem.

Comment 1 Martin Oberhuber

2011-10-04 18:11:59 EDT

I tend to see this as "invalid".

- The whole reason of having an IConnectorService is to manage the "connected"
  state of all subsystems sharing a session. The IConnectorService alone should
  be the master of the "connected" state.

- When your custom subsystem is associated with the SshConnectorService it must
  not try re-connecting when the SshConnectorService says it's connected already.
  In other words, when you override isConnected() I claim that you also have
  to override connect() appropriately.

The whole concept of having a single subsystem's connected status depend on two independent channels / connector services seems invalid to me ...

Comment 2 Simon Bernard

2011-10-05 05:39:26 EDT

(In reply to comment #1)
> - The whole reason of having an IConnectorService is to manage the "connected"
>   state of all subsystems sharing a session. The IConnectorService alone should
>   be the master of the "connected" state.
oki. (In my case I see it as the "master" connection.)
 
> - When your custom subsystem is associated with the SshConnectorService it must
>   not try re-connecting when the SshConnectorService says it's connected
> already.
>   In other words, when you override isConnected() I claim that you also have
>   to override connect() appropriately.
I'm agree and that's what I do finaly. (but the code don't allow to do that easily see my answer of Bug 359874)

> The whole concept of having a single subsystem's connected status depend on two
> independent channels / connector services seems invalid to me ...
It's a pity because, I think it's perhaps a common usecase. We do that twice in our product and it does not lack much to offer this possibility to rse users.


The way I found the problem is not really the point to focus. What I would like to show it's if for any reason (the reason below or this reason Bug 359955 or anyone else) there a two call of connect without a disconnect call between, this will create a new session (so a new thread) and the previous one will never be terminated. We could be more robust.

Comment 3 Simon Bernard

2011-10-05 07:55:10 EDT

I reread myself, I'm not very clear and my poor english doesn't really help :/.

What I'm saying is that :
For sshconnector, if connect() is called twice without a disconnect() between, we will create a Thread (session) which will never be terminated.

Theoretically, this should not happen, but for some unexpected reason (like the one above, or the one described in bug 359955, or any other reason) it could happen.

To be more robust, we could modify connect() and avoid the thread leak by :
- closing the session (and so terminating the thread) before to recreate one.
or
- avoiding to reconnect if we already are connected.