Comment 1 Sheldon 2007-05-17 10:02:42 EDT

I just wanted to know a few details, as per the current status of code the shell service launches a new shell every time a launch shell is done for the telnet service, 

The only problem here is that the multiple instances of telnet shells share the same instance of telnet client, as a result all commands of multiple shells are written to the same stream which causes a problem so an ls performed on one shell causes the output to be displayed on other shells too.

I would like to know which can be an ideal solution to solve this problem,
using a separate telnet client instance for every shell, or having some kind of session id associated with each shell which can be used to distinguish which commands have to be executed for which shell and where the output should be displayed

Comment 2 Martin Oberhuber 2007-05-17 14:56:27 EDT

I'd prefer doing a separate telnet client for each shell. Trying to multiplex commands/responses over a single channel is way too error-prone. If such a multiplexed channel should ever be needed, this should be a separate project. 

The only question is what to do with the connectorservice. Since each shell connects/disconnects individually, there is no concept of a "session connect" for telnet.

Perhaps the best is if the connectorservice just keeps track of the number of telnet clients. Goes to connected state when the first one is created, and goes to disconnected state when the last one exits. Disconnect of the subsystem (or the connectorservice) should terminate all open shells.

Comment 3 Sheldon 2007-05-18 09:57:05 EDT

Created attachment 67796 [details]
Patch for multiple shells problem

I have attached a patch for the multiple shells problem, the solution is based on what Martin has recommended.

Comment 4 Sheldon 2007-05-18 09:58:32 EDT

Hi Martin, i have attached the patch give it a try and let me know if any changes are required.

Thanks

Comment 5 Martin Oberhuber 2007-05-18 21:23:34 EDT

Thanks for the patch. Unfortunately it came in a little too late for M7, but I will consider it next week.

On my first impression, I find it strange that all login handling is done in TelnetHostShell now. This may be problematic if we'd ever want to attach a different kind of client (e.g. the terminal) with the connection. It's strange that the shell needs to know the Properties associated with the Connectorservice. 

I had thought about a solution where the actual code for the login would be done in TelnetConnectorService, but it would be executed separately in each shell context. Clients could ask the ConnectorService for an InputStream and OutputStream (rather than getting the commons.net TelnetClient instance). This would also shield the Shell (and other client) code from commons.net, and might make re-using this easier for similar protocols like rlogin.

But I didn't think this through to the end, it's just an initial impression.

Comment 6 Sheldon 2007-05-25 09:34:34 EDT

Created attachment 68766 [details]
new patch for 187301

I have attached a new patch for this bug, based on the recommendations made by martin for the previous patch

Comment 7 Sheldon 2007-05-25 09:36:30 EDT

Hi Martin i have modified the patch which provides a solution similar
to what you have recommended. Please have a look at it and let me know if modifications have to be made.

Thanks
Sheldon

Comment 8 Martin Oberhuber 2007-05-25 12:44:14 EDT

Sheldon - thanks for the patch, but I do not believe the new patch actually solves the problem. Did you test this with multiple shells? It seems to me that all the shells associated with one connection would still all use the same fTelnetClient.

Also, the ITelnetSessionProvider interface has way too many methods that I do not think are really necessary. Why don't you go with a "factory" approach:

public interface ITelnetSessionProvider {
   public TelnetClient makeNewTelnetClient() throws Exception;
}

Your TelnetConnectorService implements ITelnetSessionProvider. When the HostShell calls makeNewTelnetClient, the TelnetConnectorService creates a new instance of TelnetClient, connects it, and runs its authentication code. Failures during connect or authentication are reported as exceptions. So the client always gets an already-connected TelnetClient or needs to handle the exception. When the host shell is done (i.e. in its exit() method) you just close the TelnetClient -- it might be interesting to have the ConnectorService listen to exit() events or broken connection events, not sure if it can register for those with the TelnetClient, but I'm not sure if this is really necessary.

In the connect() method of your ConnectorService, which is understood as "connecting the session", you just run makeNewTelnetClient() in order to see if it authenticates successfully. If not, throw away the TelnetClient and mark the session as not connected.

I think the point is, that Telnet doesn't have a concept of a session which allows multiple shells. There is always just one shell. Therefore, the session concept needs to be added. We do this in the connector service, by keeping all the authentication code in there. The session provider is able to create new TelnetClients.

I'm not sure if I'll have time to invest much more in this, so I'd appreciate if you could come up with another patch. For now, I'm keeping telnet experimental and deferring this to RC2.

Comment 9 Martin Oberhuber 2007-06-06 06:40:41 EDT

Sheldon - I have no time fixing this properly myself. I've updated the 

  http://wiki.eclipse.org/index.php/TM_2.0_Known_Issues_and_Workarounds

mentioning this problem so unless you can come up with a proper patch until Monday morning my time LATEST, I see no chance getting this fixed for TM 2.0. I consider a proper patch one that has been tested to work, presumably along the lines I have outlined in comment #8.

Please base a patch off RSE HEAD or 2.0RC2.

Comment 10 Sheldon 2007-06-06 09:02:20 EDT

Created attachment 70316 [details]
new patch for 187301

Hi Martin i have attached a new patch for this bug, it is based on the comments you posted previously. I have used the factory approach for the session provider which you have recommended.
I carried out a few tests and things are working correctly, please let me know if you have any issues
Thanks

Comment 11 Sheldon 2007-06-11 14:17:25 EDT

Hi Martin i have not received any reply from you, just wanted to know if the fix i had sent is useful or it requires modification

Thanks

Comment 12 Martin Oberhuber 2007-06-26 13:46:57 EDT

Created attachment 72503 [details]
Cleaned up patch v3 - 75 LOC

Sorry for the long delay in processing this - I was on vacation.

The new patch is certainly the best I've seen so far, although there are still problems. Attach is a cleaned-up version of this patch:
  * Updated the Copyright Comments
  * Organized Imports
  * Changed order of methods in order to get minimal diffs to previous version
  * Removed
     - ITelnetSessionProvider.getTelnetClient()
     - ITelnetSessionProvider.handleSessionLost()
    and associated implementations, because in Telnet, the various shells are
    really totally independent. There is not really a concept of a "session",
    i.e. one shell does not influence the others. Thus, exiting one shell 
    must not handleSessionLost() and thus kill the others.
  * changed TelnetConnectorService to maintain a list of all connections,
    because we consider the "session" aka connectorService connected, when 
    any of the shells are connected; and other way round, disconnecting the
    session must disconnect all shells so we need a list of all clients.

Currently known issues are
 (a) because "in" and "out" are instance variables, running
     makeNewTelnetClient() from different threads in quick succession will 
     likely fail (the input and output streams would be shared between the
     threads). These variables should be passed into the connect thread 
     instead. I will open a separate bug for this.
 (b) Typing "exit" in a telnet shell does not mark this shell disconnected
     or inactive. Looks like some event handling is missing here.
 (c) Pressing the "cancel shell" or "remove shell" button in either the remote
     commands view or the remote systems tree makes the shell inactive but
     does not change its icon or change the shell's background to gray as it
     should be. Probably a dupliate of (b) -- looks like some event handling
     is missing. I'm going to file separate bug reports for (b) and (c).

Comment 13 Martin Oberhuber 2007-06-26 13:49:06 EDT

Committers - attached patch definitely improves the Telnet situation, and although I would still consider it experimental I'd like to get this patch applied for 2.0. It will definitely affect telnet only, and I have only seen positive effects on Telknet although it is not perfect yet.

All committers please approve this patch for 2.0

Comment 14 Martin Oberhuber 2007-06-26 14:03:12 EDT

(In reply to comment #12)
I filed bug #194464 in order to track issue (a) mentioned above (quick creation of multiple telnet shells), and bug #194466 in order track issues (b) and (c) (shell connected status not updated properly).

Comment 15 Sheldon 2007-06-26 14:41:48 EDT

Hi Martin, thanks for the feedback on the patch. I will have a look at the new bugs and will try to come up with fixes as soon as possible.

Comment 16 Martin Oberhuber 2007-06-27 04:28:52 EDT

Cleaned up patch committed and released for 2.0