Thread: Communicating with a Telnet server

Hi,

I'm quite new to network programming in general and I patched together some code which requests data from a server and prints that date. The code works but it doesn't look like a clean solution to me. Can I simplify the code? Especially removing the never stopping while loop and all those "else if" with the "writer.flush" look a bit redundant.

import java.io.IOException;
import java.io.InputStream;
import java.io.PrintWriter;
import java.net.Socket;
import java.net.SocketException;
import java.util.Calendar;
 
public class TelnetIO {
 
	private static final String END_KEY = "$$EOE";
	private static final String START_KEY = "$$SOE";
	private final static String CMD_EXIT = "exit\r\n";
	private String host = "horizons.jpl.nasa.gov";
	private int port = 6775;
	private String date;
 
	public static void main(String[] args) {
		TelnetIO socket1 = new TelnetIO();
		System.out.println("The final result is:\n"+socket1.getData(399));
	}
 
	private TelnetIO() {
		Calendar today = Calendar.getInstance();
		date = today.get(Calendar.YEAR)+"-"+(today.get(Calendar.MONTH)+1)+"-"+today.get(Calendar.DAY_OF_MONTH);
        System.out.println("Current date: "+date);
	}
 
	public String getData(int objectid) {
		String strdata = "ERROR";
 
		try {
			Socket telnet = new Socket(host, port);
			telnet.setKeepAlive(true);
			PrintWriter writer = new PrintWriter(telnet.getOutputStream());
			InputStream reader = telnet.getInputStream();
			StringBuilder sb = new StringBuilder();
			byte[] buffer = new byte[8192];
			int len;
			boolean bconnected = false;
 
			while ((len = reader.read(buffer)) != -1) {
				String readchar = new String(buffer, 0, len);
				sb.append(readchar + "\n");
				//System.out.println(readchar);
 
				if (bconnected && readchar.contains(START_KEY) && readchar.contains(END_KEY)) {
					strdata = readchar.substring(readchar.indexOf(START_KEY)+START_KEY.length(), readchar.indexOf(END_KEY)).trim();
				}
 
				if (readchar.contains("[E]phemeris")) {
					writer.print("E\r\n");
					writer.flush();
				}
				else if (readchar.contains("[N]ew-case")) {
					writer.print("N\r\n");
					writer.flush();
				}
				else if (readchar.toLowerCase().contains("observe")) {
					writer.print("v\r\n");
					writer.flush();
				}
				else if (readchar.toLowerCase().contains("coordinate center")) {
					writer.print("500@0 \r\n");
					writer.flush();
				}
				else if (readchar.toLowerCase().contains("starting")) {
					writer.print(date+" 00:00\r\n");
					writer.flush();
				}
				else if (readchar.toLowerCase().contains("ending")) {
					writer.print(date+" 00:01\r\n");
					writer.flush();
				}
				else if (readchar.toLowerCase().contains("output interval")) {
					writer.print("1m\r\n");
					writer.flush();
				}
				else if (readchar.toLowerCase().contains("confirm") ||
						readchar.toLowerCase().contains("accept default output") ) {
					writer.print("y\r\n");
					writer.flush();
				}
				else if (readchar.toLowerCase().contains("reference plane")) {
					writer.print("eclip\r\n");
					writer.flush();
				}
				else if (readchar.trim().endsWith("Horizons>")) {
					if(!bconnected) {
						writer.print(objectid+"\r\n");
						bconnected = true;
					}
					else {
						writer.print(CMD_EXIT);
						writer.flush();
					}
					writer.flush();
				}
				else if (bconnected) {
					//System.out.println("Unknown line: "+readchar);
				}
 
			}
			telnet.close();
		} catch (SocketException e) {
			e.printStackTrace();
		} catch (IOException e) {
			e.printStackTrace();
		}
 
		//We only want line 2, 3 and 4
		return strdata.split("\n")[1]+strdata.split("\n")[2]+strdata.split("\n")[3];
	}
}

EDIT:Some nice highlighting

Originally Posted by EoD

...Especially removing the never stopping while loop

Question: What do you think is the purpose of the never-stopping while loop?

Answer: Keeps trying to read from socket as long as the socket connection is maintained.

Question: How does it ever leave the never-stopping while loop?

Answer: After getting the last thing that the program is looking for, the program sends an "exit" command to the server's "Horizons>" application on this telnet port.

Then the server takes down the socket connection and the program detects that, leaves the loop, and prints the significant part of the payload and terminates gracefully. Of course if some network problem causes the socket connection to be broken (or prevents establishment of the connection in the first place), the program also exits that non-stopping loop and terminates (but maybe with an exception, since the program doesn't actually test for satisfactory retrieval of the information).

So: What would be your way of doing something other than the non-stopping while loop?

Well, maybe the program could be changed to implement some kind of state machine that depends on the sequence of messages from the server rather than just looking for particular substrings without regard to sequence, but would that be simpler to implement? Maybe yes or maybe no.

I do think that it might more elegant and maybe more straightforward to document and to modify if the server ever changes its tune. This might be worth pursuing if you are really interested. Once you get into the "state machine" groove, you realize that everything is a state machine. (It's like when you give a kid a hammer: Everything begins looking like a nail!)

Big complicated sequences (bigger and more complicated than this straightforward stuff) can be broken down into very simple code in the state machine engine. The state transition table (maybe not so simple) that drives the machine might be a head-scratcher, but it will all be in a single location in the program and its documentation. Starting with simple programs like this (especially having one that already works), might be a good training experience for getting into stuff the way the Big Guys do it. (Note that there still has to be some kind of loop that continuously, or at least periodically, checks for socket validity since it is going to be trying to read a stream from the server for the almost the entire time. But so what? What else is the program going to do other than monitoring and acting on responses from the server.)

Anyhow...

Originally Posted by EoD

and all those "else if" with the "writer.flush" look a bit redundant.

Well, what about all of those if/else things? Maybe you could consider a look-up table inside a function that sees if a received table entry String is a substring of a received string. Then that would allow selection of the appropriate action String. Whenever a match is found, a single place in the program could tack on the "\r\n" thing and do the flush thing.

Looking at the if-else stuff, it seems that, maybe the table could be a 2-D array of Strings like the following:

        String [][] actionTable = {
            {"[E]phemeris"          , "E"         },
            {"[N]ew-case"           , "N"         },
            {"observe"              , "v"         },
            {"coordinate center"    , "500@0 "    },
            {"starting"             , startDateStr},
            {"ending"               , endDateStr  },
            {"output interval"      , "1m"        },
            {"confirm"              , "y"         },
            {"accept default output", "y"         },
            {"reference plane"      , "eclip"     }
 
        };

Note that startDateStr and endDateStr are built at run time by appending " 00:00" and " 00:01" to a Date string obtained from Calender methods. That's do-able.

A function (maybe named String getAction(String promptString, String [][] actionTable)) would step through the table and see if actionTable[k][0] is a substring of the promptString and return the actionTable[k][1] (or some such thing). Will return the action string if a match is found, otherwise, maybe return an empty string. Or some such thing.

Note that the "Horizons>" prompt doesn't fit readily into this scheme since there are two different actions, based on whether it has been seen before during this session. Furthermore, a state variable, bconnected, is changed, depending on whether or not this is the first time. This variable affects things other than just the action that should result from finding the "Horizons>" prompt.

So, maybe the test for "Horizons>" would be done separately, as it is now. (Or you could, maybe, "enhance" the table lookup function to take care of bconnected.)

Note that it's not a simple case of pattern matching, since idea is that the first entry is a substring of the server's response that determines what the program should do. (Otherwise you could make a more sophisticated HashTable<String, String> to do the deed in a single function call rather than having to step through a 2-D array.)

Since there are a grand total of eleven or twelve action Strings, implementation efficiency is hardly an issue (not to me, at least), but some might prefer the elegance of a lookup table rather than a sequence of a dozen or so if-else statements.

My opinion on that: Chacun à son goût!



A final note: Your ability to "patch together some code" that results in a non-trivial living, breathing, working program involving network sockets is an outstanding effort! It presents a VSLO (Very Special Learning Opportunity---I like making up abbreviations like this, but some people think I'm just being silly. Oh, well...).

So...

Experiment. Go for a Better Mousetrap (I was going to abbreviate Better Mousetrap too, but...)


Cheers!

Z

Originally Posted by EoD

First of all, a huge thanks for the very detailed answer! I even smiled while reading it


It happened to me that the connection hung or my input was faulty and the program just waited forever. Is there some nice solution apart from running my own timeout timer?

Handling network timeouts

Probably the easiest way:
Default SO_TIMEOUT is zero, which means that reads are blocking. You can use the Socket.setSoTimeout() method to give a maximum delay before returning with a value of zero.

Originally Posted by EoD

...Actually I could just ignore that part and parse my own input, it should not do much overhead

The original loop simply ignores Strings from the server that don't contain the carefully selected substrings that are considered to be a prompt, so it isn't an issue if you are using the program to execute an automatic play of a previously observed "manual" session. (I assume that you have run a session with your own system's telnet client to see what the server actually gives out at each step.)

Now about the program itself: I didn't offer criticism of the logic since you said it "works," so, if it suits your purposes, you can use it as is.

However, there are a number of discrepancies that might cause erratic behavior. (At least that's what I observed.)

For example the strings from the server aren't "parsed" at all. They are merely examined to see if they contain a particular substring. Case is ignored.

I'll show an example: The original program, as well as your table-driven version treats any String with the case-ignoring substring "observe" to be the prompt that requires the client response to be the character v.

The actual prompt is

 Observe, Elements, Vectors  [o,e,v,?] :

I note that there are a number of strings from the server that contain case-ignoring "observe" but maybe that's OK as long as the substrings are tested in the correct order.

I think it would be safer to test (at least) that the actual prompt occurs. And maybe, even better, test that it occurs at or near the end of the String, using the substring method that lets the search starting at a particular index value rather than searching the entire String.
.
So, for starters, the first String for that table entry should be, perhaps, the actual prompt, not the word "observe." Then, why not force an exact match instead of doing a case-insensitive search? Then think about how to use the length of the prompt to give a starting value for the indexed substring method.

The table that I showed in my previous post was based on the if-else sequence in the original code, and was intended to be a starting point, not the last word. The idea is: Take something that works. Derive another approach and verify that it works the same way. Then, make whatever functional improvements that you discover the need for as you experiment. Having the prompt strings and response strings together in a table makes it easier to maintain and modify as the need arises.

The state machine approach would not just search every server string for a certain substring, but would start looking for the "Horizons >" prompt and issue the response.

Then it would be looking for the "Select ... [E]phemeris, [F]tp, [M]ail, [R]edisplay, ?, <cr>:" prompt and issue its response.

Etc.

Since this kind of session is linear (no loops in the state machine), I don't think the state machine approach would, in itself, simplify the logic, but this program could act as a learning tool for situations where the actual sequence is more complicated. Again, having a program that works is a great jumping-off point for further exploration.

One other thing: The final String, containing the payload, contains three sets of numbers with each set separated by a '\r' character. Maybe these print OK on some systems, but not on my Linux. (The '\r' characters cause only the last line of the three to be printed.)

So, I changed main() to this:

    public static void main(String [] args) {
        TelnetIO socket1 = new TelnetIO();
        String socketStr = socket1.getData(399);
        String outStr = "";
 
        // Change '\r' to '\n'
        for (int i = 0; i < socketStr.length(); i++) {
            char thisChar = socketStr.charAt(i);
            if (thisChar == '\r') {
                outStr += '\n';
            }
            else {
                outStr += thisChar;
            }
        }
 
        System.out.println("The final result is:\n" + outStr);
    } // End of main()

Cheers!

Z

Originally Posted by Zaphod_b

Probably the easiest way:
Default SO_TIMEOUT is zero, which means that reads are blocking. You can use the Socket.setSoTimeout() method to give a maximum delay before returning with a value of zero.

Ok, that's what I was looking for!

Originally Posted by Zaphod_b

So, for starters, the first String for that table entry should be, perhaps, the actual prompt, not the word "observe." Then, why not force an exact match instead of doing a case-insensitive search? Then think about how to use the length of the prompt to give a starting value for the indexed substring method.

That's what I meant with the "portability" above. I had the impression that Windows and Linux received different invisible characters at some (not all) lines. It also seemed that at some lines there are some weird spaces being added. I did a full match at the beginning, then tried a match at the End of a line but it didn't work out and I want to have a working code so I switched to the substring match. I'm considering to replace all invisible ascii chars from the to-be-parsed String and trim() it before doing a "full" match.
Yes, this "observere" I did expand to "observe, elements, vectors" but apparently deleted it again -.-
I'm using only lower-case matches, because I had so many typos on my matching Strings that I didn't want to worry anymore about them. Changed it back

Originally Posted by Zaphod_b

Since this kind of session is linear (no loops in the state machine), I don't think the state machine approach would, in itself, simplify the logic, but this program could act as a learning tool for situations where the actual sequence is more complicated. Again, having a program that works is a great jumping-off point for further exploration.

I'm still not quite sure how to do this properly, but I'll dig into it once I got more free time.

Originally Posted by Zaphod_b

One other thing: The final String, containing the payload, contains three sets of numbers with each set separated by a '\r' character. Maybe these print OK on some systems, but not on my Linux. (The '\r' characters cause only the last line of the three to be printed.)

I didn't observe any problems on both Debian, Gentoo and Windows while Parsing the String. But it's cleaner to remove the '\r', yeah.
EDIT: Bah, I already removed the '\r' in the code I use, stupid me! But the problem was the lack of "\n" which made problems

Almost same code as last time (should I stop posting it?)

import java.io.IOException;
import java.io.InputStream;
import java.io.InterruptedIOException;
import java.io.PrintWriter;
import java.net.Socket;
import java.net.SocketException;
import java.util.Calendar;
 
public class TelnetIO extends Thread {
 
	private static final String END_KEY = "$$EOE";
	private static final String START_KEY = "$$SOE";
	private static final String DATE_DUMMY = "xDATEx";
	private static final String strERROR = "ERROR";
	private static final double[] iERROR = {0,0,0};
	private static final String HOST = "horizons.jpl.nasa.gov";
	private static final int PORT = 6775;
	private static final int TIMEOUT = 5000;
	private String date;
	private String[][] actionTable = {
		{"[E]phemeris"                  , "e"					},
		{"[N]ew-case"                   , "n"					},
		{"Observe, Elements, Vectors"   , "v"					},
		{"Coordinate center"            , "@ssb"				},
		{"Starting CT"                  , DATE_DUMMY + " 00:00"	},
		{"Ending   CT"                  , DATE_DUMMY + " 00:01"	},
		{"Output interval"              , "1m"					},
		{"Confirm selected station"     , "y"					},
		{"Accept default output"        , "y"					},
		{"Reference plane"              , "eclip"				},
		{"Horizons>"                    , "exit"				},
	};
 
	public static void main(String[] args) {
		TelnetIO socket1 = new TelnetIO();
		String data = socket1.getData(399);
		System.out.println("The final result is:\n" + data);
		/* ignore next line, just for completeness :) */
		System.out.println("The z coordinate is: " + transformIntoCoords(data)[2]);
	}
 
	private TelnetIO() {
		Calendar today = Calendar.getInstance();
		date = today.get(Calendar.YEAR)+"-"+(today.get(Calendar.MONTH)+1)+"-"+today.get(Calendar.DAY_OF_MONTH);
		System.out.println("Current date: "+date);
 
		for(int i=0; i<actionTable.length; i++)
			if(actionTable[i][1].contains(DATE_DUMMY))
				actionTable[i][1] = actionTable[i][1].replaceAll(DATE_DUMMY, date);
	}
 
	public String getData(int objectid) {
		String strdata = strERROR;
 
		try {
			Socket telnet = new Socket(HOST, PORT);
			telnet.setKeepAlive(true);
			telnet.setSoTimeout(TIMEOUT);
			PrintWriter writer = new PrintWriter(telnet.getOutputStream());
			InputStream reader = telnet.getInputStream();
			byte[] buffer = new byte[8192];
			int len;
 
			boolean bconnected = false;
 
			try {
				while ((len = reader.read(buffer)) != -1) {
					String readchar = new String(buffer, 0, len);
					System.out.println(readchar);
 
					/* already inside the chain of command? */
					if(bconnected) {
						if (readchar.contains(START_KEY) && readchar.contains(END_KEY)) {
							/* received data */
							strdata = readchar.substring(readchar.indexOf(START_KEY)+START_KEY.length(), readchar.indexOf(END_KEY)).trim();
						}
						else {
							//System.out.println(readchar);
							/* server probably wants interaction */
							for(int i=0; i<actionTable.length; i++) {
								if(readchar.contains(actionTable[i][0])) {
									writer.print(actionTable[i][1] + "\r\n");
									writer.flush();
									break;
								}	
							}
						}
					}
					else if (readchar.trim().endsWith("Horizons>")) {
						writer.print(objectid + "\r\n");
						writer.flush();
						bconnected = true;
					}
 
				}
			} catch (InterruptedIOException e) {
				System.out.println("Timeout reached. Cancelling!");
			} finally {
				telnet.close();
			}
		} catch (SocketException e) {
			e.printStackTrace();
		} catch (IOException e) {
			e.printStackTrace();
		}
 
		// We only want line 2, 3 and 4
		if(strdata.split("\n").length < 4)
			return strERROR;
		else
			return strdata.split("\r\n")[1] + "\n" + strdata.split("\r\n")[2] + "\n" + strdata.split("\r\n")[3];
	}
 
	/* ignore the following code - not network related */
	static double[] transformIntoCoords(String data) {
		String[] values = data.split("\n");
		System.out.println("transformIntoCoords() values[0]: '"+values[0]+"'");
		String[] coords = values[0].trim().replaceAll("[ ]+", " ").split(" ");
		if(coords.length < 3) {
			System.out.println("transformIntoCoords() - ERROR - coords.length < 3");
			return iERROR;
		}
 
		double[] xxx = iERROR;
		for(int i=0; i<xxx.length; i++)	{
			System.out.print("transformIntoCoords() coords["+i+"]: '"+coords[i]+"'  -->  ");
			try {
				xxx[i] = Double.parseDouble(coords[i]);
			} catch (NumberFormatException e) {
				System.out.println("Error parsing value");
				xxx = iERROR;
				break;
			}
			System.out.println(xxx[i]);
		}
		return xxx;
	}
}