views:

647

answers:

2

I am writing a webservice that allows users to post files and then retrieve them at a URL (basically think of it as the RESTful Amazon S3). The issue I came across was rather then return a byte[] from my Oracle query (Spring JDBC) I am returning an InputStream and then streaming the data back to the client in chunks. This (IMO) is a much better idea since I put no size restriction on the file and I don't want 2GB byte arrays in memory.

At first it seemed to work fine, but I ran into a case during heavy load that sometimes a Connection would get reused before the previous servlet could send the file. It seems after the JDBC call that returned the InputStream, the Connection would be returned to the pool (Spring would call conn.close(), but not clear the associated ResultSet). So if no other request was given that Connection then the InputStream would still be valid and could be read from, but if the Connection was given to a new request then the InputStream would be null and the previous request would fail.

My solution was to create a subclass of InputStream that also takes a Connection as a constructor arg, and in the overridden public close() method also close the Connection. I had to ditch the Spring JDBC and just make a normal PreparedStatement call, otherwise Spring would always return the connection to the pool.

public class ConnectionInputStream extends InputStream {

   private Connection conn;
   private InputStream stream;

   public ConnectionInputStream(InputStream s, Connection c) {
      conn = c;
      stream = s;
   }

   // all InputStream methods call the same method on the variable stream

   @Override
   public void close() throws IOException {
      try {
         stream.close();
      } catch (IOException ioex) {
          //do something
      } finally {
         try {
             conn.close();
         } catch (SQLException sqlex) {
             //ignore
         }
      }
   }
}

Does anyone have a more elegant solution, or see any glaring problems with my solution? Also this code wasn't cut/paste from my actual code so if there is a typo just ignore it.

A: 

Why not read the entire InputStream/byte[]/whatever from the query before releasing the query yourself? It sounds like you are trying to return data from the query after your code has told Spring / the pool that you are done with the connection.

matt b
Because the InputStream is being passed back to the controller from the DAO. Once Spring JDBC completes a call it immediately calls connection.close(). I'm not going to pass the ServletResponse all the way into the data access layer.
Gandalf
What I meant was, read the contents of the InputStream completely from the resultset/connection to some sort of intermediate datatype, and then return a new InputStream wrapping that datatype. This way you read all of the contents of the file prior to closing the connection.
matt b
In other words, I don't think you can return a reference to an InputStream (which will read from an underlying connection) and then close the connection prior to reading the stream
matt b
But that would completely invalidate the point of returning the Inputstream. If I read the entire contents of the InputStream then I have just [possibly] loaded 2GB of data into memory - exactly what I do not want to do. I want to write out 10K chunks of the file and have the client read them from the the Outputstream of the response.
Gandalf
+1  A: 

Unfortunately, my imagination went wild when you asked this question. I don't know if this solution is considered more elegant. However, these classes are simple and easily re-usable so you may find a use for them if they are not satisfactory. You will see everything coming together at the end...

public class BinaryCloseable implements Closeable {

    private Closeable first;
    private Closeable last;

    public BinaryCloseable(Closeable first, Closeable last) {
     this.first = first;
     this.last = last;
    }

    @Override
    public void close() throws IOException {
     try {
      first.close();
     } finally {
      last.close();
     }
    }

}

BinaryCloseable is used by CompositeCloseable:

public class CompositeCloseable implements Closeable {

    private Closeable target;

    public CompositeCloseable(Closeable... closeables) {
     target = new Closeable() { public void close(){} };
     for (Closeable closeable : closeables) {
      target = new BinaryCloseable(target, closeable);
     }
    }

    @Override
    public void close() throws IOException {
     target.close();
    }

}

The ResultSetCloser closes ResultSet objects:

public class ResultSetCloser implements Closeable {

    private ResultSet resultSet;

    public ResultSetCloser(ResultSet resultSet) {
     this.resultSet = resultSet;
    }

    @Override
    public void close() throws IOException {
     try {
      resultSet.close();
     } catch (SQLException e) {
      throw new IOException("Exception encountered while closing result set", e);
     }
    }

}

The PreparedStatementCloser closes PreparedStatement objects:

public class PreparedStatementCloser implements Closeable {

    private PreparedStatement preparedStatement;

    public PreparedStatementCloser(PreparedStatement preparedStatement) {
     this.preparedStatement = preparedStatement;
    }

    @Override
    public void close() throws IOException {
     try {
      preparedStatement.close();
     } catch (SQLException e) {
      throw new IOException("Exception encountered while closing prepared statement", e);
     }
    }

}

The ConnectionCloser closes Connection objects:

public class ConnectionCloser implements Closeable {

    private Connection connection;

    public ConnectionCloser(Connection connection) {
     this.connection = connection;
    }

    @Override
    public void close() throws IOException {
     try {
      connection.close();
     } catch (SQLException e) {
      throw new IOException("Exception encountered while closing connection", e);
     }
    }

}

We now refactor your original InputStream idea into:

public class ClosingInputStream extends InputStream {

    private InputStream stream;
    private Closeable closer;

    public ClosingInputStream(InputStream stream, Closeable closer) {
     this.stream = stream;
     this.closer = closer;
    }

    // The other InputStream methods...

    @Override
    public void close() throws IOException {
     closer.close();
    }

}

Finally, it all comes together as:

new ClosingInputStream(
  stream,
  new CompositeCloseable(
    stream,
    new ResultSetCloser(resultSet),
    new PreparedStatementCloser(statement),
    new ConnectionCloser(connection)
   )
 );

When this ClosingInputStream's close() method is called, this is effectively what happens (with exception handling omitted for clarity's sake):

public void close() {
    try {
        try {
            try {
                try {
                    // This is empty due to the first line in `CompositeCloseable`'s constructor
                } finally {
                    stream.close();
                }
            } finally {
                resultSet.close();
            }
        } finally {
            preparedStatement.close();
        }
    } finally {
        connection.close();
    }
}

You're now free to close as many Closeable objects as you like.

Adam Paynter
It would probably be easier just to pass a list (stack, array, whatever) of Closeables and close them all, rather then needing it to be specific to Preparestatement, Connection, etc. Also when you close a Connection it is supposed to close all underlying resources associated with that connection (such as Statements and ResultSets). I do appreciate the effort though, your code has a similar fell to my minimal example.
Gandalf
Going to accept this answer, as in the end this is almost exactly the same way I solved this issue.
Gandalf