Buffered for your convenience

pacohope

Here's some code, authored by grad students, for a Java-based network server. Although it was written by students, they were working under contract to a commercial software company to produce a prototype for real software.

if (command.equals("GET LOG"))
{
  String logContent="";
  char[] buf=new char[5*1024];  // 5 Mb buf for reading.
  try
  {
    FileReader fr=new FileReader(Server.logFile);
    int bytesRead;

   while ((bytesRead=fr.read(buf))!=-1)
      logContent+=new String(buf,0,bytesRead);

   fr.close();
}
catch (FileNotFoundException e)
{
  System.err.println("Error opening log file: Log file does not exist."+e);
}

ObjectOutputStream oos=new ObjectOutputStream(clientSocket.getOutputStream());
oos.writeObject(logContent);
oos.close();
oos.close();

Let's see. What are some of the errors?

It reads the entire log file into memory
It puts the log file contents into a 5K buffer (not 5Mb, as the comment suggests) before moving them to a String
It reallocates a new string each time it reads a bit more from the file
After doing all this ridiculous stuff, it writes the entire stream to the output socket of the other side in one go
They double close the socket. Let's make sure it's good and closed!
The clientSocket was passed in as an argument to this function. It was open when we got it, but we'll close it as a side-effect. Dubious.
They catch one exception: file not found. What about I/O exceptions while reading? What about exceptions while writing to the socket? What about HolyCrapTenGigabyteLogFilesDontFitInRAMExceptions?

And if the log file is larger than 5K? It's almost like these guys were trying to write a buffer overflow in Java, but just couldn't quite figure out how. :)

bobday

I think later versions of the Java yoke will optimize those concatenations into a StringBuffer behind the scenes, but the whole mess is still inexcusable.

rbowes

@pacohope said:

They catch one exception: file not found. What about I/O exceptions while reading? What about exceptions while writing to the socket? What about HolyCrapTenGigabyteLogFilesDontFitInRAMExceptions?

Not to mention true and false!

@pacohope said:

authored by grad students

I think I found your problem

VGR

And let's not overlook creating a String from bytes without specifying the charset. Sure hope the machine's default locale isn't UTF-8.

eimaj2nz

@pacohope said:

Let's see. What are some of the errors?

It reads the entire log file into memory

It puts the log file contents into a 5K buffer (not 5Mb, as the comment suggests) before moving them to a String

It reallocates a new string each time it reads a bit more from the file

After doing all this ridiculous stuff, it writes the entire stream to the output socket of the other side in one go

They double close the socket. Let's make sure it's good and closed!

The clientSocket was passed in as an argument to this function. It was open when we got it, but we'll close it as a side-effect. Dubious.

They catch one exception: file not found. What about I/O exceptions while reading? What about exceptions while writing to the socket? What about HolyCrapTenGigabyteLogFilesDontFitInRAMExceptions?

And if the log file is larger than 5K? It's almost like these guys were trying to write a buffer overflow in Java, but just couldn't quite figure out how. :)

Regarding your points above...

Yes, reading the log file into memory could be an issue if it's huge. You can only assume here that the log files are a reasonable length.
The buffer isn't supposed to be big enough to fit the entire file. It's used again and again to read chunks of the file.
I don't know Java all that well, but I would assume that the string construction used here is a standard way to turn an array of chars (bytes???) into a string.
I agree with another poster that appending to a string isn't that optimal, but the compiler may decide to use a string buffer instead.
Writing the entire log to the socket in one go isn't always a bad thing, again assuming that in most cases the log file will be small. Passing the data in one big block allows the system to decide how big each packet will be, without waiting for a coalescing algorithm to do its thing.
Yes, double close is definitely not needed, unless there's some very odd behaviour we don't know about.
They're not closing the socket, just the stream writer which they've attached to the socket. I would assume that this doesn't also affect the underlying output stream.
While they catch just the one exception type in this method, there's likely to be an exception handler at a higher level. It's quite possible that "file not found" is a special case that shouldn't be passed to the higher-level handler.

All that being said, it is quite inefficient to read the data into memory then immediately write that data to a stream. A better option would have been something like:

byte[] buffer = new byte[4096]; // 4KB buffer
int bytesRead = 0;
FileReader fr=new FileReader(Server.logFile);
while ((bytesRead = fr.read(buffer)) != -1)
clientSocket.getOutputStream().write(buffer, 0, bytesRead);

poochner

That's not a 5Kb buffer, it's a 10Kb buffer, but most likely it will be handled as 5K Latin1 chars, since that seems to be the "default default" charset these days.

They're not just writing the data out to the client socket, they're serializing it (ObjectOutputStream). To do that properly, they have to have the whole thing in one object (or write it as a sequence of other objects, with some sort of terminating flag, blah, blah, blah, complicated, blah).

They don't have a space between their part of the error message and the exception's toString(). That's tacky.

If the file isn't found, they just close the client socket. Poor client, sitting there waiting on a java.lang.String to come down the pipe and just hears a loud CLANG! from the other end getting shut. Since they're sending objects, they could send the exception. It'd be weird, but who cares? Just imagine the code you have on the client end to deal with "and then I read a FileNotFoundException..." "You mean it threw a FNFE?" "No, I read one successfully." "WTF!?"

aquanight

@VGR said:

And let's not overlook creating a String from bytes without specifying the charset. Sure hope the machine's default locale isn't UTF-8.

Incorrect. The string is created from an array of char. char != byte. In fact, a char is 2 bytes (described range is consistant for unsigned 16-bit - making char the only unsigned type in java) and would presumably obey default locale (never messed with locale myself, so I dunno), and the FileReader's read method here is in fact reading characters. And then they make a String from that array. So if the default local is UTF-8, they're still ok because FileReader will read in UTF-8, the character array will store in UTF-8, and the String will be UTF-8. If the default locale is russian, same deal. FileReader reads in russian, char[] array in russian, String in russian.

Einsidler

@aquanight said:

If the default locale is russian, same deal. FileReader reads in russian, char[] array in russian, String in russian.

But in Soviet Russia, FileReader reads YOU!!