(File)OutputStreams and their usage

Discussion in 'Java' started by Philipp, May 16, 2008.

  1. Philipp

    Philipp Guest

    Dear all,

    Is this (see code) the correct way of handling a FileOutputStream?
    Specific question are in the code. Thanks for your answers. Philipp

    public void load(File file){
    OutputStream os;
    try {
    os = new FileOutputStream(file);
    } catch (Exception e) {
    logger.log("Could not open file output stream", e);
    // can os be non-null here?
    // should I put a close() here?
    return;
    }

    try {
    load(os); // call of another load method with OutputStream
    } catch (Exception e) {
    logger.log("Exception while loading from file.", e);
    return; // is this return of any interest?
    } finally {
    if(os != null){
    try {
    os.close();
    } catch (Exception e) {
    // exception while closing, what can we do?
    }
    }
    }
    }
    Philipp, May 16, 2008
    #1
    1. Advertising

  2. Philipp wrote:
    > Dear all,
    >
    > Is this (see code) the correct way of handling a FileOutputStream?
    > Specific question are in the code. Thanks for your answers. Philipp
    >
    > public void load(File file){
    > OutputStream os;
    > try {
    > os = new FileOutputStream(file);
    > } catch (Exception e) {
    > logger.log("Could not open file output stream", e);
    > // can os be non-null here?
    > // should I put a close() here?
    > return;
    > }
    >
    > try {
    > load(os); // call of another load method with OutputStream
    > } catch (Exception e) {
    > logger.log("Exception while loading from file.", e);
    > return; // is this return of any interest?
    > } finally {
    > if(os != null){
    > try {
    > os.close();
    > } catch (Exception e) {
    > // exception while closing, what can we do?
    > }
    > }
    > }
    > }
    >


    Well, you're converting from exceptions to error codes.
    load() can silently fail and it's up to the caller to check if
    it has loaded anything. I would propagate exceptions or convert them to
    another type of exception.

    --
    Simplicity is the ultimate sophistication.
    -- Leonardo da Vinci
    Leonard Milcin, May 16, 2008
    #2
    1. Advertising

  3. Philipp

    Philipp Guest

    Leonard Milcin wrote:
    > Philipp wrote:
    >> Dear all,
    >>
    >> Is this (see code) the correct way of handling a FileOutputStream?
    >> Specific question are in the code. Thanks for your answers. Philipp
    >>
    >> public void load(File file){
    >> OutputStream os;
    >> try {
    >> os = new FileOutputStream(file);
    >> } catch (Exception e) {
    >> logger.log("Could not open file output stream", e);
    >> // can os be non-null here?
    >> // should I put a close() here?
    >> return;
    >> }
    >>
    >> try {
    >> load(os); // call of another load method with OutputStream
    >> } catch (Exception e) {
    >> logger.log("Exception while loading from file.", e);
    >> return; // is this return of any interest?
    >> } finally {
    >> if(os != null){
    >> try {
    >> os.close();
    >> } catch (Exception e) {
    >> // exception while closing, what can we do?
    >> }
    >> }
    >> }
    >> }
    >>

    >
    > Well, you're converting from exceptions to error codes.
    > load() can silently fail and it's up to the caller to check if
    > it has loaded anything. I would propagate exceptions or convert them to
    > another type of exception.


    Yes, you are correct, I should definitely rethrow rather than log at
    that point.
    But this was not really my question. I'm rather asking at what points I
    I have to call close() on the stream to gurantee correct release of
    resources in all cases and whether having a return in the first or
    second catch is problematic in this respect.
    Phil
    Philipp, May 16, 2008
    #3
  4. Philipp wrote:
    > Leonard Milcin wrote:
    >> Philipp wrote:
    >>> Dear all,
    >>>
    >>> Is this (see code) the correct way of handling a
    >>> FileOutputStream? Specific question are in the code. Thanks for
    >>> your answers. Philipp
    >>>
    >>> public void load(File file){ OutputStream os; try { os = new
    >>> FileOutputStream(file); } catch (Exception e) { logger.log("Could
    >>> not open file output stream", e); // can os be non-null here? //
    >>> should I put a close() here? return; }
    >>>
    >>> try { load(os); // call of another load method with OutputStream
    >>> } catch (Exception e) { logger.log("Exception while loading from
    >>> file.", e); return; // is this return of any interest? }
    >>> finally { if(os != null){ try { os.close(); } catch (Exception e)
    >>> { // exception while closing, what can we do? } } } }
    >>>

    >>
    >> Well, you're converting from exceptions to error codes. load() can
    >> silently fail and it's up to the caller to check if it has loaded
    >> anything. I would propagate exceptions or convert them to another
    >> type of exception.

    >
    > Yes, you are correct, I should definitely rethrow rather than log at
    > that point. But this was not really my question. I'm rather asking at
    > what points I I have to call close() on the stream to gurantee
    > correct release of resources in all cases and whether having a return
    > in the first or second catch is problematic in this respect. Phil


    public void load(File file) throws ... {
    OutputStream os;
    try {
    os = new FileOutputStream(file);
    load(os);
    } finally {
    if (os!=null) {
    os.close();
    }
    }
    }

    That looks much cleaner. The caller has to deal with all those checked
    excetions, though. You can convert to another type of exception (like
    unchecked exception).

    The reason why I don't surround os.close() with try/catch is that
    usually it should not throw exception but if it does... I would
    certainly want to know. Besides, load(os) probably also throws
    IOException and nested try/catch looks too ugly for my taste...

    When it goes to logging you should do logging only when you deal with
    exception. Perhaps also when it crosses some boundary (like library,
    layer, etc.) but, personally, I don't like it.

    Regards,
    Leonard

    --
    Simplicity is the ultimate sophistication.
    -- Leonardo da Vinci
    Leonard Milcin, May 16, 2008
    #4
  5. Philipp

    Roedy Green Guest

    On Fri, 16 May 2008 13:06:59 +0200, Philipp <>
    wrote, quoted or indirectly quoted someone who said :

    >Is this (see code) the correct way of handling a FileOutputStream?
    >Specific question are in the code. Thanks for your answers. Philipp


    Don't catch an exception before you are ready to deal with it. Just
    propagate it up to the caller if it is not obvious to you what to do.


    Here is a cleanup trick:

    finally {
    if ( handle != null ) handle.close();
    }

    IIRC Java now closes your files if you forget to on exit.
    If you crash, you are SOL.

    Could someone please verify that, and ideally tell us when that
    behaviour changed or was it always or never that way.
    --

    Roedy Green Canadian Mind Products
    The Java Glossary
    http://mindprod.com
    Roedy Green, May 16, 2008
    #5
  6. Philipp

    Philipp Guest

    Lew wrote:

    > Catch-log-and-rethrow is a better idiom, or declare a return type that
    > can indicate failure if client code cares not for the reason.


    FWIW catch-log-and-rethrow is considered an exception handling
    anti-pattern here:
    http://today.java.net/pub/a/today/2006/04/06/exception-handling-antipatterns.html

    They suggest to either rethrow or log, but not both.


    > try
    > {
    > OutputStream os = new FileOutputStream(file);
    > workWithGuaranteedNonNullValue( os );
    > // factored method that handles its own exceptions
    > os.close();
    > }
    > catch ( IOException exc )
    > {
    > logger.error( IOERROR_MSG, exc );
    > }
    >
    > It is easy to add a return value, with success from the try{} or failure
    > from the catch{}.
    >


    Yeah I will try that.

    Phil
    Philipp, May 16, 2008
    #6
  7. Philipp

    Philipp Guest

    Philipp wrote:
    > Lew wrote:


    >
    >> try
    >> {
    >> OutputStream os = new FileOutputStream(file);
    >> workWithGuaranteedNonNullValue( os );
    >> // factored method that handles its own exceptions
    >> os.close();
    >> }
    >> catch ( IOException exc )
    >> {
    >> logger.error( IOERROR_MSG, exc );
    >> }


    Hmm... Isn't it so, that if workWithGuaranteedNonNullValue( os ); throws
    an exception, the stream will not be closed? In which case you will have
    to move the close to a finally block (and test os for null again...). Am
    I missing something here?

    Phil
    Philipp, May 16, 2008
    #7
  8. Philipp

    Philipp Guest

    Roedy Green wrote:
    > On Fri, 16 May 2008 13:06:59 +0200, Philipp <>
    > wrote, quoted or indirectly quoted someone who said :
    >
    >> Is this (see code) the correct way of handling a FileOutputStream?
    >> Specific question are in the code. Thanks for your answers. Philipp

    >
    > Don't catch an exception before you are ready to deal with it. Just
    > propagate it up to the caller if it is not obvious to you what to do.


    OK.

    > Here is a cleanup trick:
    >
    > finally {
    > if ( handle != null ) handle.close();
    > }


    FWIW, at the following site they recommand:
    http://today.java.net/pub/a/today/2006/04/06/exception-handling-antipatterns.html

    "If the code that you call in a finally block can possibly throw an
    exception, make sure that you either handle it, or log it. Never let it
    bubble out of the finally block."

    So we should surround the .close() with a try-catch and log something in
    the catch. (this is why I did it in my original post)

    > IIRC Java now closes your files if you forget to on exit.
    > If you crash, you are SOL.


    Isn't the OS taking care of that for you (on exit of the JVM)?

    Phil
    Philipp, May 16, 2008
    #8
  9. Philipp

    Philipp Guest

    Philipp wrote:
    > Philipp wrote:
    >> Lew wrote:

    >
    >>
    >>> try
    >>> {
    >>> OutputStream os = new FileOutputStream(file);
    >>> workWithGuaranteedNonNullValue( os );
    >>> // factored method that handles its own exceptions
    >>> os.close();
    >>> }
    >>> catch ( IOException exc )
    >>> {
    >>> logger.error( IOERROR_MSG, exc );
    >>> }


    > Am I missing something here?


    Yeah, I am. Obviously workWithGuaranteedNonNullValue() will not throw
    IOException...
    Sorry for the monolog.

    Phil
    Philipp, May 16, 2008
    #9
  10. Philipp

    Roedy Green Guest

    On Fri, 16 May 2008 15:46:14 +0200, Philipp <>
    wrote, quoted or indirectly quoted someone who said :

    >Isn't the OS taking care of that for you (on exit of the JVM)?


    But that would not flush your final buffer full that the OS knows
    nothing about.
    --

    Roedy Green Canadian Mind Products
    The Java Glossary
    http://mindprod.com
    Roedy Green, May 16, 2008
    #10
  11. Philipp

    Tom Anderson Guest

    On Fri, 16 May 2008, Leonard Milcin wrote:

    > Philipp wrote:
    >> Leonard Milcin wrote:
    >>> Philipp wrote:
    >>>
    >>>> Is this (see code) the correct way of handling a FileOutputStream?
    >>>> Specific question are in the code. Thanks for your answers.
    >>>
    >>> Well, you're converting from exceptions to error codes. load() can
    >>> silently fail and it's up to the caller to check if it has loaded
    >>> anything. I would propagate exceptions or convert them to another type of
    >>> exception.

    >>
    >> Yes, you are correct, I should definitely rethrow rather than log at
    >> that point. But this was not really my question. I'm rather asking at
    >> what points I I have to call close() on the stream to gurantee correct
    >> release of resources in all cases and whether having a return in the
    >> first or second catch is problematic in this respect.

    >
    > public void load(File file) throws ... {
    > OutputStream os;
    > try {
    > os = new FileOutputStream(file);
    > load(os);
    > } finally {
    > if (os!=null) {
    > os.close();
    > }
    > }
    > }
    >
    > That looks much cleaner.


    It does. But it doesn't compile.

    Something everyone has missed here - god alone knows how, since it's
    pretty bloody basic - is that if the initialisation of a variable fails
    due to an exception, then that variable is uninitialised, and it can't be
    used. Plus, if you're somewhere where there's a chance that a variable
    might be uninitialised, you aren't allowed to use it.

    That means that your finally clause is illegal - it could be reached
    following an exception in "new FileOutputStream(file)", and so the
    variable os has to be treated as potentially uninitialised, and so
    unusable.

    Philipp's original code had this situation too - where he asks "can os be
    non-null here? should I put a close() here?". The answer is that os can't
    be non-null, and it also can't be null - it's uninitialized. So it's not
    so much that you don't need to check for a non-null os, or close it, as
    much as that you can't.

    So to finally address what i think Philipp is asking: you can't clean up a
    failed FileOutputStream, but happily, you don't need to: it's the
    constructor's job to clean up after itself before throwing an exception.
    Hopefully, it's actually doing this.

    In Leonard's code, there's a simple fix: change the declaration of os to
    be an initialisation:

    OutputStream os = null ;

    Then everything works as it should.

    I'm dubious about the close() in the finally block not being wrapped in a
    try-catch; if i get an IO error during loading, i want to see that, not
    some subsequent exception that arose when trying to close the file. I'd
    wrap it in a try-catch and log or ignore any exceptions.

    There's actually a yet slicker way to write this method:

    public void load(File file) throws IOException {
    OutputStream os = new FileOutputStream(file) ;
    try {
    load(os) ;
    }
    finally {
    try {
    os.close() ;
    }
    catch (IOException e) {
    // log or ignore
    }
    }
    }

    You put the FileOutputStream constructor outside the try-finally block;
    you know that if it fails, there's no stream to close, so there's no
    reason to have it inside. That means you can drop the test for null.

    > The caller has to deal with all those checked excetions, though. You can
    > convert to another type of exception


    Yes.

    > (like unchecked exception).


    NO!

    > The reason why I don't surround os.close() with try/catch is that
    > usually it should not throw exception but if it does... I would
    > certainly want to know.


    Very sound advice.

    To answer Philipp's other question, the return in your second catch block
    is pointless. With or without it, execution will go through the finally
    block and then leave the method.

    I'll add a question of my own: why are we loading from an *output* stream?

    tom

    --
    It's the 21st century, man - we rue _minutes_. -- Benjamin Rosenbaum
    Tom Anderson, May 16, 2008
    #11
  12. Tom Anderson wrote:
    > I'm dubious about the close() in the finally block not being wrapped in
    > a try-catch; if i get an IO error during loading, i want to see that,
    > not some subsequent exception that arose when trying to close the file.
    > I'd wrap it in a try-catch and log or ignore any exceptions.
    >
    > There's actually a yet slicker way to write this method:
    >
    > public void load(File file) throws IOException {
    > OutputStream os = new FileOutputStream(file) ;
    > try {
    > load(os) ;
    > }
    > finally {
    > try {
    > os.close() ;
    > }
    > catch (IOException e) {
    > // log or ignore
    > }
    > }
    > }


    Well, to summarize things, we end up with:

    public void load(File file) throws IOException {
    OutputStream os = new FileOutputStream(file);
    try {
    load(os);
    } finally {
    os.close();
    }
    }

    > I'll add a question of my own: why are we loading from an *output* stream?


    Perhaps we're loading *into* output stream.


    Regards,
    Leonard

    --
    Simplicity is the ultimate sophistication.
    -- Leonardo da Vinci
    Leonard Milcin, May 16, 2008
    #12
  13. Lew wrote:
    > Leonard Milcin wrote:
    >> public void load(File file) throws ... {
    >> OutputStream os;
    >> try {
    >> os = new FileOutputStream(file);
    >> load(os);
    >> } finally {
    >> if (os!=null) {
    >> os.close();
    >> }
    >> }
    >> }

    >
    > The check for os non-nullity is not needed here.


    I believe that is true. But checking refs for null in finally
    before calling close on them is generally a good practice.
    It is not needed here, but getting it in the fingers could
    be a good thing-

    > Because of that, you
    > can move the close() inside the try{} block.


    I don't think so - in that case it will not be called if load throws
    an exception.

    > because
    > close() can throw an Exception, too.


    That would need to be handled to get a robust application.

    > The idiom above is a bit too loose. It doesn't log, it doesn't
    > translate the Exception into the application domain, and it allows
    > Exceptions to happen in the finally{} block. It also doesn't admit of a
    > coherent Exception-handling strategy throughout the application.


    Not necessarily.

    It is bad practice to catch at every level in the call stack.

    So if this is the outer level in a layer, then it does have poor
    exception handling.

    But if it is any other level, then it is just fine.

    (except for the exception in close problem)

    Arne
    Arne Vajhøj, May 17, 2008
    #13
  14. Philipp wrote:
    > Lew wrote:
    >> Catch-log-and-rethrow is a better idiom, or declare a return type that
    >> can indicate failure if client code cares not for the reason.

    >
    > FWIW catch-log-and-rethrow is considered an exception handling
    > anti-pattern here:
    > http://today.java.net/pub/a/today/2006/04/06/exception-handling-antipatterns.html
    >
    > They suggest to either rethrow or log, but not both.


    Yes, but the argument against it is pretty weak.

    "Logging and throwing results in multiple log messages for a single
    problem in the code, and makes life hell for the support engineer who is
    trying to dig through the logs."

    I can not see that as a real problem. Not with a 50 line log
    file and not with a 100 MB log file.

    If you are troubleshooting then you need all the info and
    seeing a log entry both where it happened and where it
    is handles should be beneficial.

    Arne
    Arne Vajhøj, May 17, 2008
    #14
  15. Philipp

    Philipp Guest

    Leonard Milcin wrote:

    <snip>
    Thanks for that explanation.

    >> I'll add a question of my own: why are we loading from an *output*
    >> stream?


    Yeah sorry I got my load and save methods mixed up. It should be
    InputStreams instead.

    Phil
    Philipp, May 17, 2008
    #15
  16. Philipp

    Tom Anderson Guest

    On Fri, 16 May 2008, Leonard Milcin wrote:

    > Tom Anderson wrote:
    >
    >> I'm dubious about the close() in the finally block not being wrapped in a
    >> try-catch; if i get an IO error during loading, i want to see that, not
    >> some subsequent exception that arose when trying to close the file. I'd
    >> wrap it in a try-catch and log or ignore any exceptions.
    >>
    >> There's actually a yet slicker way to write this method:
    >>
    >> public void load(File file) throws IOException {
    >> OutputStream os = new FileOutputStream(file) ;
    >> try {
    >> load(os) ;
    >> }
    >> finally {
    >> try {
    >> os.close() ;
    >> }
    >> catch (IOException e) {
    >> // log or ignore
    >> }
    >> }
    >> }

    >
    > Well, to summarize things, we end up with:
    >
    > public void load(File file) throws IOException {
    > OutputStream os = new FileOutputStream(file);
    > try {
    > load(os);
    > } finally {
    > os.close();
    > }
    > }


    I don't like the unprotected close() in the finally block. But if you're
    happy with it, then yes.

    tom

    --
    I didn't think, "I'm going to change the world." No, I'm just going to
    build the best machines I can build that I would want to use in my own
    life. -- Woz
    Tom Anderson, May 17, 2008
    #16
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Yurai Núñez Rodríguez

    Problems using ASPNET OutputStreams

    Yurai Núñez Rodríguez, May 2, 2004, in forum: ASP .Net
    Replies:
    0
    Views:
    283
    Yurai Núñez Rodríguez
    May 2, 2004
  2. Andy Fish

    closing writers and outputstreams

    Andy Fish, Nov 13, 2003, in forum: Java
    Replies:
    1
    Views:
    487
    Thomas Weidenfeller
    Nov 13, 2003
  3. Bruce Lee
    Replies:
    1
    Views:
    342
    Steve Horsley
    Dec 7, 2004
  4. Knute Johnson

    Re: which OutputStreams are buffered?

    Knute Johnson, May 16, 2008, in forum: Java
    Replies:
    1
    Views:
    324
    Knute Johnson
    May 16, 2008
  5. Tom Anderson

    Re: which OutputStreams are buffered?

    Tom Anderson, May 16, 2008, in forum: Java
    Replies:
    9
    Views:
    438
    Tom Anderson
    May 21, 2008
Loading...

Share This Page