Community
Participate
Working Groups
The Telnet Terminal Service to be implemented with bug 240523 needs to set Telnet Options such as ECHO, SuppressGA, TerminalType and WindowSize. Apache Commons Net requires these options to be set before the connection is actually connected. The current ITelnetSessionProvider doesn't allow this, because session creation and login procedure are tied together. New API should be created, which allows to create the Commons.Net TelnetClient externally, configure it with options as needed and connect it, and only *then* pass it into the ITelnetSessionProvider / ConnectorService in order to have the login procedure done.
Created attachment 120939 [details] Patch with a first draft Attached patch adds the following new API: interface ITelnetSessionProvider { public TelnetClient loginTelnetClient( TelnetClient client, IProgressMonitor monitor) throws SystemMessageException; }; clients can now create and configure the TelnetClient as needed, then give it to the ITelnetSessionProvider for doing the login handling. This implements the original request, but still leaves room for improvement: 1.) In case of cancellation, the code disconnects the TelnetClient and returns NULL, whereas in other cases of communication error it does not disconnect. This is not consistent. 2.) Is it even necessary to have the alien class "TelnetClient" in the API signature? -- Given that the TelnetClient needs to be configured and connected externally, it seems to make more sense to just pass the InputStream and the OutputStream into the API to process the login. This approach would give the chance of re-using the pattern matching code for the login for other protocols such as rlogin or serial. Attached patch is committed: [259367][telnet][api] Allow the ITelnetSessionProvider to configure Telnet options before logging in rse.services.telnet and rse.connectorservice.telnet have been reved up to 1.2 since new API has been added. Patch is released for TM 3.1m4.
We should decide on a final API until 3.1m5.
(In reply to comment #1) > 2.) Is it even necessary to have the alien class "TelnetClient" in the API > signature? -- Given that the TelnetClient needs to be configured and > connected externally, it seems to make more sense to just pass the > InputStream and the OutputStream into the API to process the login. > This approach would give the chance of re-using the pattern matching > code for the login for other protocols such as rlogin or serial. Right now ITelnetSessionProvider contains two methods: makeNewTelnetClient and loginTelnetClient, and both of them have TelnetClient as a returned object. Does it make sense to eliminate TelnetClient only from loginTelnetClient?
How about removing makeNewTelnetClient method from ITelnetSessionProvider at all? Given the fact that the TelnetClient needs to be configured and connected externally?
When the kind of connection is "simple" like the plain shell without terminal support, no configuration of the TelnetClient is necessary and the TelnetClient can be obtained in a simple way without clients having to know the gory details. Initially I had somehow hoped that the connectorService interface could even abstract away from using Apache Commons as the Telnet implementation, so I'm not too happy about having TelnetClient in the API at all. Just exposing the Streams through the API seemed better, then we can change the Telnet implementation if we need. But so far nobody had the time to look at this in any more detail, so the simplest possible solution is what we have now. The API is "internal" so we can change whatever we want easily. If you feel like removing something in order to improve the situation, feel free to remove it.
I'd suggest removing makeNewTelnetClient from ITelnetSessionProvider, but leaving it in TelnetConnectorService, and then passing streams to loginTelnetClient. I'm not sure yet whether loginTelnetClient should be void or return boolean(success/failure) -- I need to think about it more. How do you like this plan?
Couple questions on your strategy: * Who would be creating the telnet connection and how? * Who would be the "owner" of the telnet connection, i.e. responsible for sending notifications / updating UI status when the session gets lost? In terms of return value, I've never been a fan of boolean returns since they don't explain exact enough what's the reason for the failure. Users expect different notifications for issues such as canceled, timeout, wrong password, host not found etc.etc. In my opinion such verbose failure reporting is best accomplished by throwing exceptions. Using exceptions, however, the method can return void because when no exception is thrown you know that the method worked successfully.
(In reply to comment #7) > Couple questions on your strategy: > > * Who would be creating the telnet connection and how? > * Who would be the "owner" of the telnet connection, i.e. responsible for > sending notifications / updating UI status when the session gets lost? I'd say class implementing ITelnetSessionProvider? > In terms of return value, I've never been a fan of boolean returns since they > don't explain exact enough what's the reason for the failure. Users expect > different notifications for issues such as canceled, timeout, wrong password, > host not found etc.etc. In my opinion such verbose failure reporting is best > accomplished by throwing exceptions. Using exceptions, however, the method > can return void because when no exception is thrown you know that the method > worked successfully. Agreed.
(In reply to comment #8) > I'd say class implementing ITelnetSessionProvider? I'm afraid I've lost your thread of thought somehow... I'm sure I'll understand when I see some code, so I'd say move forward and draft it :-)
Isn't this committed already? API Freeze is near..
(In reply to comment #10) > Isn't this committed already? API Freeze is near.. No, not yet, cause I figured out that to get rid of TelnetClient in loginTelnetClient implementation, we need to move out the following line: client.connect(getHostName(), getTelnetPort()); which means that we need to have these methods in ITelnetSessionProvider. I'm not sure if this is a good solution.
(In reply to comment #11) > No, not yet, cause I figured out that to get rid of TelnetClient in > loginTelnetClient implementation, we need to move out the following line: Hm... I'm not sure if I fully understand. Note that when you want to get rid of class TelnetClient from the API (which I think is a good thing!) and provide Streams only to the outside (which I also think is a good thing!), then the API needs to provide methods that allow configuring the connection as needed. In particular, the code in TelnetTerminalShell line 90ff needs to go into TelnetConnectorService (or other class implementing ITelnetSessionProvider): - should there be Echo or not? - how to negotiate about window size, ie set colument/rows on the connection? - how to specify the requested terminal type? - I'm not exactly sure that what the "SUPPRESS_GO_AHEAD" Telnet option means but you might want to be able to switch this on or off in the API as well - How would you return both InputStream and OutputStream? If you really want to go along this route and get rid of Commons Net from the API, I guess you'll need a surrogate of the TelnetClient, which has only those very few methods that you need -- and you may better check the TelnetClient original methods to see if there don't happen to be any more: /** @noimplement @noextend */ interface ITelnetSession { public InputStream getInputStream(); public OutputStream getOutputStream(); public boolean isConnected(); //not sure if you need this; the idea //would be that it wraps your current EOFDetectingInputStreamWrapper public void setTerminalSize(int widthCols, int heightRows, int widthPix, int heightPix); public boolean isLocalEcho(); //query negotiation result public String getTerminalType(); //query negotiation result public int getRows(); //query negotiation result public int getColumns(); //query negotiation result } and then do this: interface ITelnetSessionProvider { ITelnetSession makeSession (String host, int port, boolean doEcho, String ptyType); }
(In reply to comment #12) On second thought... one drawback of my proposal from comment #11 is that it hard-codes the options that are available for configuring to Echo, PtyType and WindowSize only. I'm not sure if any other options would ever make sense. But anyways, here's an idea how it could be kept extensible: (1) Allow the ITelnetSession to be configured *before* connecting it (2) Add generic setters / getters for telnet option constants: /** @noimplement @noextend */ interface ITelnetSession { public InputStream getInputStream(); public OutputStream getOutputStream(); public boolean isConnected(); //not sure if you need this; the idea //would be that it wraps your current EOFDetectingInputStreamWrapper public void connect() throws Exception; //option setters / getters for convenience public void setLocalEcho(boolean b); public boolean isLocalEcho(); //query negotiation result public void setTerminalType (String s); public String getTerminalType(); //query negotiation result public void setTerminalSize(int widthCols, int heightRows, int widthPix, int heightPix); public int getRows(); //query negotiation result public int getColumns(); //query negotiation result //generic option setters and getters public void setTelnetOption(int optionType, String value); public String getTelnetOption(int optionType); } interface ITelnetSessionProvider { //echo and ptyType to be configured after making the session ITelnetSession makeSession (String host, int port), } What I generally dislike about this approach is that as our API gets larger and larger, we tend to re-implement Apache TelnetClient. As the ITelnetSessionProvider is "internal" anyways I don't quite see the benefit of this... or are there plans to make the ITelnetSessionProvider public? In fact, with a public API similar to this, we could have an "ITerminalSessionProvider" that's not necessarily limited to telnet, but the very same API could be used for SSH as well. That would be interesting again, since it would probably allow us to get rid of the JSch classes from API as well... and thus eventually make the kind of SSH implementation replaceable... Anna, what do you really intend to complete with this bug by 3.1m6 ?
PPS rather than thinking about generic API for an ITerminalSessionProvider (which would duplicate work of API design), I think it would make more sense to invest that time into building a bridge to run on top of existing ECF APIs. The ECF project is the canonical place for generic APIs for communications at Eclipse. They have approached us before asking to build RSE services on top of their generic provider APIs. They have existing API for what we call connector service (connected/disconnected status handling; they call it "Container"): http://www.eclipse.org/ecf/documentation.php API for generic addressing (instead of URL: they call it "Namespace" and "Identity"); existing API for file transfer providers: http://www.eclipse.org/ecf/org.eclipse.ecf.docs/api/org/eclipse/ecf/filetransfer/service/package-frame.html For Terminal-like sessions like Telnet and SSH shells, the ECF Datashare API may be good enough, perhaps some adapter would be needed for the terminal type / window size requests, but the Input and Output Streams would be an ECF listener or ECF sendMessage() respectively since ECF is asynchronous: http://www.eclipse.org/ecf/org.eclipse.ecf.docs/api/org/eclipse/ecf/provider/datashare/BaseChannel.html
(In reply to comment #13) Getting realistic again and looking at the original intent and description of this bug, I think that actually the crucial thing here is being able to *create a session before connecting it*, such that options can be set on the session before connecting it. In the SSH case, we have this kind of behavior because the JSch Session class can be obtained (and configured) before connecting it. In the Telnet case, we have it committed today, because the TelnetClient class an be obtained (and configured) before connecting it. What's not beautiful in both cases is that our API exposes the underlying libraries. Which is not a very big deal since it is internal anyways; and on the positive side gives us full flexibility. We could make it happen and move towards a generic API like ITerminalSessionProvider / ITerminalSession, or even IRSESessionProvider / IRSESession, and it might be an interesting exercise. The code in comment #12 may be helpful to go in that direction. But again, how much time and effort do we want to put into this, given that 3.1m6 marks the API freeze and is next week already? Our current API is not public anyways, and works as today. So we could also defer this. Anna what do you think?
If working in this area, I'd prefer working on bug 267474 ... since I believe that from its implementation we'd likely learn more about what any ITerminalSession API requires...
(In reply to comment #15) > (In reply to comment #13) > But again, how much time and effort do we want to put into this, given that > 3.1m6 marks the API freeze and is next week already? Our current API is not > public anyways, and works as today. So we could also defer this. Anna what do > you think? Hi Martin, I just wanted to complete this task as it is. Actually I didn't really figure out that it's internal API. I'd be happy to defer it and get back to it later if you are okay with it.
ITelnetSessionProvider can do today what we need today, and it's not even public API. No need to look at improving this right now, may consider again in the future.
