3

any comments on my code on allowing user to download file. 

if(fileObject !=null)
response.setHeader("Content-disposition", "attachment; filename=\""+fileObject.getFilename()+"\"");
response.setContentType(fileObject.getFiletype());
response.setContentLength((int)fileObject.getFilesize().intValue());
try {
 if(response !=null && response.getOutputStream() !=null &&fileObject!=null && fileObject.getBinData() !=null ){
    OutputStream out = response.getOutputStream();
    out.write(fileObject.getBinData());
 }


} catch (IOException e) {
    throw new ApplicationRuntimeException(e);
}

most of the time, i dont get below error. but once and a while, i get error

29 Nov 2010 10:50:41,925 WARN [http-2020-2] - Unable to present exception page: getOutputStream() has already been called for this response
java.lang.IllegalStateException: getOutputStream() has already been called for this response
 at org.apache.catalina.connector.Response.getWriter(Response.java:610)
pstanton
  • 35,033
  • 24
  • 126
  • 168
cometta
  • 35,071
  • 77
  • 215
  • 324
  • you state that this has something to do with tapestry, however there is no reference to anything tapestry in your question. please explain how this is tapestry related or remove the tag. – pstanton Nov 29 '10 at 04:04

4 Answers4

4

The exception message is clear:

Unable to present exception page: getOutputStream() has already been called for this response
java.lang.IllegalStateException: getOutputStream() has already been called for this response
at org.apache.catalina.connector.Response.getWriter(Response.java:610)

An IOException was been thrown and you're rethrowing it as a custom exception which forced the servletcontainer to show the exception page which will use getWriter() for this. You should in fact let any IOException go because that's usually a point of no return.

An IOException can for example be thrown during the job when the client aborted the request. The best practice is to not catch IOException on the Servlet API yourself. It's already declared in throws clause of the servlet methods.

protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException {
    FileObject fileObject = getItSomehow();
    if (fileObject != null && fileObject.getBinData() != null) {
        response.setHeader("Content-disposition", "attachment; filename=\"" + fileObject.getFilename() + "\"");
        response.setContentType(fileObject.getFiletype());
        response.setContentLength((int)fileObject.getFilesize().intValue());
        response.getOutputStream().write(fileObject.getBinData());
    } else {
        // ???
    }
}
BalusC
  • 1,082,665
  • 372
  • 3,610
  • 3,555
3

You are calling response.getOutputStream() twice. Instead, call it once and assign it to a local variable, then use that variable for your null check and your write operation.

try {
 OutputStream out = response.getOutputStream();
 if(response !=null && out !=null &&fileObject!=null && fileObject.getBinData() !=null ){
    out.write(fileObject.getBinData());
 }
} catch (IOException e) {
  throw new ApplicationRuntimeException(e);
}
highlycaffeinated
  • 19,729
  • 9
  • 60
  • 91
  • 4
    That's not the cause of the problem. You can call `getOutputStream()` and `getWriter()` each as many times as you want, but **not both** on the *same* response. – BalusC Nov 29 '10 at 03:13
0

How can response be null? Especially after you've already used it? Or response.getOutputStream()? Or fileObject, after you've already tested it for being non-null? And used it? These tests may be doing more harm than good.

user207421
  • 305,947
  • 44
  • 307
  • 483
0

I always defer to well tested code when I can ...

http://commons.apache.org/fileupload/apidocs/index.html?org/apache/commons/fileupload/util/package-summary.html

doylecentral
  • 1