Java Sockets passing between methods

Question

I am just starting out with Java Socket Programming, and I have been reading literature on sockets here. The below code is a sample from a textbook I've taken which asks me to find the bug. Comparing with the literature though I am not seeing any bugs. The creation of the socket, bufferedreader, and printwriter seem correct, and they are surrounded in a try-catch block as well. The are properly "close()"ed in a try-catch block as well. Is there an error when passing these to process()? Any help would be appreciated.

import java.net.*;
import java.io.*;

class main{

public void process(PrintWriter out, BufferedReader in, Socket echoSocket){
//....
}

public void processData() {
    Socket echoSocket;
    PrintWriter out;
    BufferedReader in;
    try{
        echoSocket = new Socket("server.company.com", 8081);
        out = new PrintWriter(echoSocket.getOutputStream(), true);
        in = new BufferedReader(new InputStreamReader(echoSocket.getInputStream()));
    }
    catch (Exception e) {
        System.err.println("Exception has occured");
        return;
    }
    process(out, in, echoSocket);
    try {
        out.close();
        in.close();
        echoSocket.close();
    }
    catch(IOException e) {
        System.err.println("IOException has occurred.");
    }
  }
}

Answer 1

Although, typos notwithstanding, one can only guess what the actual "bug" is, this code has an issue with error handling. Specifically, in the disposal of resources.

Discussion about resources

What are resources ?

Basically : any Java Object that relies on underlying OS level resources. Mostly : IO resources (input and output streams, channels), Sockets. But more importantly : if the "thing" you're using has a close, dispsose, shutdown or any of the like, it surely holds on to resources internally.
There are some exceptions (notably ByteArrayInputStream holds no resource but memory), but these are implementation details : if you stick to their interface (and you should, this is a "contract"), every stream should be closed.
Since Java 7, most of these objects in the Java API implement the AutoCloseable interface, but many 3rd parties have not necessarily ported this to their code (and maybe some can't for other reasons).

As one of the code reviewers at my company : I stop reading and I reject any code as soon as I do not see a secure call to the close method of a resource. By secure I mean inside a finally clause, that is guaranteed to be executed.

Rule of thumb about resources

Any resource obtained by your program should be freed in a finally clause (some even add : of its own).

What is the typical lifecycle of a resource Well:

1. You obtain it
2. You use it
3. You release it

In your code, that is

ResourceObject myObject = null;
try {
    myObject = getResource();
    processResource(myObject);
} finally {
    if(myObject != null) {
        try {
            myObject.close();
        } catch (Exception e) {
            // Usually there is nothing one can do but log
        }
    }
}

Since Java 7, if the resource object implements AutoCloseableyou have a new way of writing that, it's called the "try with resources".

try(ResourceObject myObject = getResource()) {
    process(myObject);
}

You do not see the finally, but it's there, the compiler writes the finally clause for you in that case.

What about multiple resources ?

Well : multiple resources, multiple finallys. The idea is to separate the causes of failures in different finally clauses. Say you want to copy a file...

public void myCopy() throws IOException {
InputStream source = null;
    try {
    source = new FileInputStream("yourInputFile");
        // If anything bad happens, I have a finally clause that protects this now   
        OutputStream destination = null;
    try {
        destination = new FileOutputStream("yourOurputFile"); // If fails, my Input will be closed thanks to its own finally
            performCopy(source, destination); // If this fail, my destination will also be closed thanks to its own finally
        } finally {
            if(destination!=null) { try { destination.close(); } catch (Exception e) {/* log*/ }}
        }
    } finally {
        if(source!=null) { try { source.close(); } catch (Exception e) {/* log*/ }}
    }
}

Or, with Java 7 syntax, we have the shorter (disclaimer : I have no Java7 right now, so can't really check if this compiles) :

try(
    InputStream input = new FileInputStream("in");
    OutputStream output = new FileOutputStream("out")) {
    performCopy(input, output);
} catch(IOException e) {
    // You still have to deal with it of course.
}

This is SO MUCH BOILERPLATE !

Yes it is. That's why we have libraries. One could argue you should not write such code. Use standard, well behaved libraries like commons IO, or use one of their utility methods. Or newer JDK methods like the Files API, and see how this works.

Commons IO has a handy IOUtils.closeQuietly() suite of methods for closing streams.

Try with resources Gotchas

There are ramifications in the "try with resources" call that go a bit deeper than that. These include: What if I want to do something with the exceptions that occur in the finally clause ? How do I differentiate that from an exception that would have occured during performCopy? Another case is : what happens here :

try(Reader reader = new InputStreamReader(new FileInputStream("in"), "an encoding that is not supported")) {
  // Whatever
}

It happens that an UnsupportedEncodingException is thrown but after the FileInputStream is instanciated. But as the FileInputStream is not the subject of the try clause, it will NOT be closed. An you have a File descriptor leak. Try that a thousand times, and your JVM will not be able to open files anymore, you OS will tell you "max number of open files exceeded" (ulimit generally does that in UNIX)

Back to your sockets

So what are your resources ?

Well, first, we can notice that you have only ONE true resource, your Socket instance, because the Socket javadoc says (javadoc):

 * <p> Closing this socket will also close the socket's
 * {@link java.io.InputStream InputStream} and
 * {@link java.io.OutputStream OutputStream}.

So your Input and Output streams are tied to your Socket, and this is enough.

What's wrong with your code

Adding comments one your original code:

try{
    echoSocket = new Socket("server.company.com", 8081);
    out = new PrintWriter(echoSocket.getOutputStream(), true); // This can throw IOException
    in = new BufferedReader(new InputStreamReader(echoSocket.getInputStream())); // Ditto
}
catch (Exception e) {
    // If an exception was thrown getting any of the streams, we get there
    System.err.println("Exception has occured");
    // And you return without closing the socket. It's bad !
    return;
}
// Let's assume everything worked, no exception.
process(out, in, echoSocket); // This may throw an exception (timeout, socket closed by peer, ...) 
                              // that is uncaught (no catch clause). Your socket will be left unclosed as a result.
try {
    out.close();              // This can fail
    in.close();               // This too
    echoSocket.close();       // And this too - although nothing you can do about it
}
catch(IOException e) {
    // if out.close fails, we get here, and in.close and socket.close 
    // never got a chance to be called. You may be leaking resources 
    System.err.println("IOException has occurred.");
}

A safe implementation

Socket echoSocket = null;
try {
    // open socket, 
    echoSocket = new Socket("server.company.com", 8081); // protected by finally
    out = new PrintWriter(echoSocket.getOutputStream(), true); // protected
    in = new BufferedReader(new InputStreamReader(echoSocket.getInputStream())); // protected
     process(out, in, echoSocket); // Still protected
} catch (Exception e) {
    // Your current error handling
} finally {
    // Anyway, this close will be called if needs be.
    if(echoSocket != null) { 
        try { echoSocket.close(); } catch (Exception e) { /* log */}
        // See javadoc, this has closed the in & out streams too.
    }
}

Answer 2

public void process(){PrintWriter out, BufferedReader in, Socket echoSocket){

should be

public void process(PrintWriter out, BufferedReader in, Socket echoSocket){

otherwise everything seems fine to me

Answer 3

Try this I think you missed one semicolon

public void processData() {
Socket echoSocket;
PrintWriter out;
BufferedReader in;
try{
    echoSocket = new Socket("localhost", 8080);
    out = new PrintWriter(echoSocket.getOutputStream(), true);
    in = new BufferedReader(new InputStreamReader(echoSocket.getInputStream()));
}
catch (IOException e) {
    System.err.println("Exception has occured");
    return;
}
process(out, in, echoSocket);
try {
    out.close();
    in.close();
    echoSocket.close();
}
catch(IOException e) {
    System.err.println("IOException has occurred.");
}


}
public void process (PrintWriter out,  BufferedReader in, Socket echoSocket)
{

}