Closing decorated streams

Discussion in 'Java' started by Philipp, Jul 9, 2009.

  1. Philipp

    Philipp Guest

    Hello, I use the following code but am not sure that it is the best or
    correct way to close the stream. In particular, if url.openStream()
    succeeds I have an open stream, but if any of the two other
    constructors throws an exception, reader will be null and the stream
    will remain open even after the finally block.

    public class WhatToClose {
    public static void main(String[] args) {
    BufferedReader reader = null;
    try{
    URL url = new URL("http://www.yahoo.com/");
    reader = new BufferedReader(new InputStreamReader(url.openStream
    ()));

    // use reader here
    String line = null;
    while (( line = reader.readLine()) != null){
    System.out.println(line);
    }
    } catch (Exception e) {
    e.printStackTrace();
    } finally {
    if(reader != null){
    try{
    reader.close();
    } catch (IOException e) {
    e.printStackTrace();
    }
    }
    }
    }
    }


    I could allocate three refs (see below) buts that's really a PITA. How
    do you do it?

    public static void main(String[] args) {
    InputStream is = null;
    InputStreamReader isr = null;
    BufferedReader br = null;
    try{
    URL url = new URL("http://www.yahoo.com/");
    is = url.openStream();
    isr = new InputStreamReader(is);
    br = new BufferedReader(isr);
    String line = null;
    while (( line = br.readLine()) != null){
    System.out.println(line);
    }
    } catch (Exception e) {
    e.printStackTrace();
    } finally {
    if(br != null){
    try{
    br.close();
    } catch (IOException e) {
    e.printStackTrace();
    }
    }
    if(isr != null){
    try{
    isr.close();
    } catch (IOException e) {
    e.printStackTrace();
    }
    }
    if(is != null){
    try{
    is.close();
    } catch (IOException e) {
    e.printStackTrace();
    }
    }
    }
    }

    Thanks Phil
    Philipp, Jul 9, 2009
    #1
    1. Advertising

  2. Philipp wrote:
    > Hello, I use the following code but am not sure that it is the best or
    > correct way to close the stream. In particular, if url.openStream()
    > succeeds I have an open stream, but if any of the two other
    > constructors throws an exception, reader will be null and the stream
    > will remain open even after the finally block.
    >
    > public class WhatToClose {
    > public static void main(String[] args) {
    > BufferedReader reader = null;
    > try{
    > URL url = new URL("http://www.yahoo.com/");
    > reader = new BufferedReader(new InputStreamReader(url.openStream
    > ()));
    >
    > // use reader here
    > String line = null;
    > while (( line = reader.readLine()) != null){
    > System.out.println(line);
    > }
    > } catch (Exception e) {
    > e.printStackTrace();
    > } finally {
    > if(reader != null){
    > try{
    > reader.close();
    > } catch (IOException e) {
    > e.printStackTrace();
    > }
    > }
    > }
    > }
    > }
    >
    >
    > I could allocate three refs (see below) buts that's really a PITA. How
    > do you do it?
    >
    > public static void main(String[] args) {
    > InputStream is = null;
    > InputStreamReader isr = null;
    > BufferedReader br = null;
    > try{
    > URL url = new URL("http://www.yahoo.com/");
    > is = url.openStream();
    > isr = new InputStreamReader(is);
    > br = new BufferedReader(isr);
    > String line = null;
    > while (( line = br.readLine()) != null){
    > System.out.println(line);
    > }
    > } catch (Exception e) {
    > e.printStackTrace();
    > } finally {
    > if(br != null){
    > try{
    > br.close();
    > } catch (IOException e) {
    > e.printStackTrace();
    > }
    > }
    > if(isr != null){
    > try{
    > isr.close();
    > } catch (IOException e) {
    > e.printStackTrace();
    > }
    > }
    > if(is != null){
    > try{
    > is.close();
    > } catch (IOException e) {
    > e.printStackTrace();
    > }
    > }
    > }
    > }
    >
    > Thanks Phil


    For normal streams, closing the top one is fine and will close the
    others. I seem to recall some issue with URLConnection streams however.
    I think there was a recent thread.

    --

    Knute Johnson
    email s/nospam/knute2009/

    --
    Posted via NewsDemon.com - Premium Uncensored Newsgroup Service
    ------->>>>>>http://www.NewsDemon.com<<<<<<------
    Unlimited Access, Anonymous Accounts, Uncensored Broadband Access
    Knute Johnson, Jul 9, 2009
    #2
    1. Advertising

  3. Philipp

    Lew Guest

    On Jul 9, 9:07 am, Philipp <> wrote:
    > Hello, I use the following code but am not sure that it is the best or
    > correct way to close the stream. In particular, if url.openStream()
    > succeeds I have an open stream, but if any of the two other
    > constructors throws an exception, reader will be null and the stream
    > will remain open even after the finally block.
    >
    > public class WhatToClose {
    >   public static void main(String[] args) {
    >     BufferedReader reader = null;


    There's another idiom that skips the 'null' assignment - see below.

    >     try{
    >       URL url = new URL("http://www.yahoo.com/");
    >       reader = new BufferedReader(new InputStreamReader(url.openStream
    > ()));
    >
    >       // use reader here
    >       String line = null;


    No need to superfluously assign a spurious 'null' to 'line'.

    >       while (( line = reader.readLine()) != null){
    >         System.out.println(line);
    >       }
    >     } catch (Exception e) {
    >       e.printStackTrace();
    >     } finally {
    >       if(reader != null){
    >         try{
    >           reader.close();


    You could just trap and close the URL stream rather than the reader.

    >         } catch (IOException e) {
    >           e.printStackTrace();
    >         }
    >       }
    >     }
    >   }
    >
    > }
    >
    > I could allocate three refs (see below) buts that's really a PITA. How
    > do you do it?
    >
    > public static void main(String[] args) {
    >   InputStream is = null;
    >   InputStreamReader isr = null;
    >   BufferedReader br = null;
    >


    This should not be necessary.

    One way is to chain the I/O classes in separate try-catch blocks, so
    that a later nullity check is unnecessary; you can use 'assert'
    statements instead. The result is verbose but precise control over
    what happens and how it's logged, and the safety of assertably
    initialized streams and readers.

    public static void main( String [] args )
    {
    final InputStream is;
    try
    {
    is = url.openStream();
    }
    catch ( IOException exc )
    {
    logger.error( "Cannot open URL stream. " + exc.getMessage() );
    return;
    }
    assert is != null;

    final BufferedReader br;
    try
    {
    br = new BufferedReader( new InputStreamReader( is ));
    }
    catch ( IOException exc )
    {
    logger.error( "Cannot open URL stream reader. " + exc.getMessage
    () );
    try
    {
    is.close();
    }
    catch ( IOException closex )
    {
    logger.error( "Cannot close URL stream. " + closex.getMessage
    () );
    }
    return;
    }
    assert br != null;

    try
    {
    for ( String line; (line = br.readLine()) != null; )
    {
    System.out.println(line);
    }
    }
    catch ( IOException exc )
    {
    logger.error( "Cannot read URL stream reader. " + exc.getMessage
    () );
    }
    finally
    {
    try
    {
    br.close();
    }
    catch ( IOException closex )
    {
    logger.error( "Cannot close URL stream reader. " +
    closex.getMessage() );
    }
    }
    }

    --
    Lew
    Lew, Jul 9, 2009
    #3
  4. Lew wrote:
    > On Jul 9, 9:07 am, Philipp <> wrote:
    >> Hello, I use the following code but am not sure that it is the best or
    >> correct way to close the stream. In particular, if url.openStream()
    >> succeeds I have an open stream, but if any of the two other
    >> constructors throws an exception, reader will be null and the stream
    >> will remain open even after the finally block.
    >>
    >> public class WhatToClose {
    >> public static void main(String[] args) {
    >> BufferedReader reader = null;

    >
    > There's another idiom that skips the 'null' assignment - see below.
    >
    >> try{
    >> URL url = new URL("http://www.yahoo.com/");
    >> reader = new BufferedReader(new InputStreamReader(url.openStream
    >> ()));
    >>
    >> // use reader here
    >> String line = null;

    >
    > No need to superfluously assign a spurious 'null' to 'line'.
    >
    >> while (( line = reader.readLine()) != null){
    >> System.out.println(line);
    >> }
    >> } catch (Exception e) {
    >> e.printStackTrace();
    >> } finally {
    >> if(reader != null){
    >> try{
    >> reader.close();

    >
    > You could just trap and close the URL stream rather than the reader.
    >
    >> } catch (IOException e) {
    >> e.printStackTrace();
    >> }
    >> }
    >> }
    >> }
    >>
    >> }
    >>
    >> I could allocate three refs (see below) buts that's really a PITA. How
    >> do you do it?
    >>
    >> public static void main(String[] args) {
    >> InputStream is = null;
    >> InputStreamReader isr = null;
    >> BufferedReader br = null;
    >>

    >
    > This should not be necessary.
    >
    > One way is to chain the I/O classes in separate try-catch blocks, so
    > that a later nullity check is unnecessary; you can use 'assert'
    > statements instead. The result is verbose but precise control over
    > what happens and how it's logged, and the safety of assertably
    > initialized streams and readers.
    >
    > public static void main( String [] args )
    > {
    > final InputStream is;
    > try
    > {
    > is = url.openStream();
    > }
    > catch ( IOException exc )
    > {
    > logger.error( "Cannot open URL stream. " + exc.getMessage() );
    > return;
    > }
    > assert is != null;
    >
    > final BufferedReader br;
    > try
    > {
    > br = new BufferedReader( new InputStreamReader( is ));
    > }
    > catch ( IOException exc )
    > {
    > logger.error( "Cannot open URL stream reader. " + exc.getMessage
    > () );
    > try
    > {
    > is.close();
    > }
    > catch ( IOException closex )
    > {
    > logger.error( "Cannot close URL stream. " + closex.getMessage
    > () );
    > }
    > return;
    > }
    > assert br != null;
    >
    > try
    > {
    > for ( String line; (line = br.readLine()) != null; )
    > {
    > System.out.println(line);
    > }
    > }
    > catch ( IOException exc )
    > {
    > logger.error( "Cannot read URL stream reader. " + exc.getMessage
    > () );
    > }
    > finally
    > {
    > try
    > {
    > br.close();
    > }
    > catch ( IOException closex )
    > {
    > logger.error( "Cannot close URL stream reader. " +
    > closex.getMessage() );
    > }
    > }
    > }
    >
    > --
    > Lew


    Lew:

    That's a pattern I've not seen before and I really like it, especially
    the for loop for reading the lines. The return in the catch block is
    pretty nifty too.

    I am curious though if you really think that all those levels are really
    necessary? The only place that can throw an I/O exception is the
    URL.getInputStream() method and if it succeeds then the BufferedReader
    constructor will succeed. So closing the BufferedReader will suffice.

    --

    Knute Johnson
    email s/nospam/knute2009/

    --
    Posted via NewsDemon.com - Premium Uncensored Newsgroup Service
    ------->>>>>>http://www.NewsDemon.com<<<<<<------
    Unlimited Access, Anonymous Accounts, Uncensored Broadband Access
    Knute Johnson, Jul 10, 2009
    #4
  5. Philipp

    Tom Anderson Guest

    On Thu, 9 Jul 2009, Philipp wrote:

    > Hello, I use the following code but am not sure that it is the best or
    > correct way to close the stream. In particular, if url.openStream()
    > succeeds I have an open stream, but if any of the two other
    > constructors throws an exception, reader will be null and the stream
    > will remain open even after the finally block.


    In this case, i'd exploit the knowledge that the upper-layer streams are
    lightweight, and don't really need to be closed - if they aren't closed
    but are garbage collected, that's fine. So i'd do:

    InputStream urlStream = url.openStream();
    try {
    BufferedReader reader = new BufferedReader(new InputStreamReader(urlStream));
    // use reader
    }
    finally {
    urlStream.close();
    }

    In the more general case, where you might have another heavyweight stream
    that needs to be explicitly closed in the stack, i'd think about keeping a
    pointer to the topmost stream, so i could just close that. Since that
    stream could actually be a reader, i'd make the variable a Closeable. Like
    so:

    InputStream urlStream = url.openStream();
    Closeable topStream = urlStream;
    try {
    Reader isr = new InputStreamReader(urlStream)'
    topStream = isr;
    BufferedReader reader = new BufferedReader(isr);
    topStream = reader;
    // use reader here
    }
    finally {
    topStream.close();
    }

    Yes, you have to write out each construction as a separate statement, and
    then add a line to update topStream. But at least you only have to do the
    closing once. Note that you actually only have to do the separation and
    updating for each heavyweight stream in the stack - lightweight streams
    can be constructed inline. Like (assuming GZIPInputStream uses libgzip
    with a native context object - i could be wrong about that):

    InputStream urlStream = url.openStream();
    Closeable topStream = urlStream;
    try {
    InputStream gz = new GZIPInputStream(new BufferedInputStream(urlStream));
    topStream = gz;
    BufferedReader reader = new BufferedReader(new InputStreamReader(gz));
    // use reader here
    }
    finally {
    topStream.close();
    }

    tom

    --
    I have been trying to find a way of framing this but yes, a light meal is
    probably preferable to a heavy one under the circumstances. -- ninebelow
    Tom Anderson, Jul 10, 2009
    #5
  6. Philipp

    Lew Guest

    Lew wrote:
    >> One way is to chain the I/O classes in separate try-catch blocks, so
    >> that a later nullity check is unnecessary; you can use 'assert'
    >> statements instead. The result is verbose but precise control over
    >> what happens and how it's logged, and the safety of assertably
    >> initialized streams and readers.
    >>
    >> public static void main( String [] args )
    >> {
    >> final InputStream is;
    >> try
    >> {
    >> is = url.openStream();
    >> }
    >> catch ( IOException exc )
    >> {
    >> logger.error( "Cannot open URL stream. " + exc.getMessage() );
    >> return;
    >> }
    >> assert is != null;
    >>
    >> final BufferedReader br;
    >> try
    >> {
    >> br = new BufferedReader( new InputStreamReader( is ));
    >> }
    >> catch ( IOException exc )
    >> {
    >> logger.error( "Cannot open URL stream reader. " + exc.getMessage
    >> () );
    >> try
    >> {
    >> is.close();
    >> }
    >> catch ( IOException closex )
    >> {
    >> logger.error( "Cannot close URL stream. " + closex.getMessage
    >> () );
    >> }
    >> return;
    >> }
    >> assert br != null;
    >>
    >> try
    >> {
    >> for ( String line; (line = br.readLine()) != null; )
    >> {
    >> System.out.println(line);
    >> }
    >> }
    >> catch ( IOException exc )
    >> {
    >> logger.error( "Cannot read URL stream reader. " + exc.getMessage
    >> () );
    >> }
    >> finally
    >> {
    >> try
    >> {
    >> br.close();
    >> }
    >> catch ( IOException closex )
    >> {
    >> logger.error( "Cannot close URL stream reader. " +
    >> closex.getMessage() );
    >> }
    >> }
    >> }


    Knute Johnson wrote:
    > That's a pattern I've not seen before and I really like it, especially
    > the for loop for reading the lines. The return in the catch block is
    > pretty nifty too.


    The return in the catch block is necessary to ensure the asserted invariant.

    > I am curious though if you really think that all those levels are really
    > necessary? The only place that can throw an I/O exception is the
    > URL.getInputStream() method and if it succeeds then the BufferedReader
    > constructor will succeed. So closing the BufferedReader will suffice.


    I agree with you in this case. So the first try block would handle:

    final BufferedReader br;
    try
    {
    br = new BufferedReader( new InputStreamReader( url.openStream() ));
    }
    catch ( IOException exc )
    {
    logger.error( "Cannot open URL stream reader. " + exc.getMessage() );
    return;
    }
    assert br != null;

    The next try block would actually use 'br' and close it in its finally block.

    The example I gave first was to address the OP's specific concern that the
    reader assignments might fail separately from the stream retrieval, and to
    illustrate the flexibility of the technique. Notice how easily the idiom
    refactors to increase or decrease level of detail and granularity of control
    by simply splitting out or merging try-catch blocks. The key to the idiom is
    that one or more try-catch blocks establish an invariant of a valid resource
    handle (the stream or reader instance), and a separate try-finally block (with
    possible catch clauses) uses that guaranteed resource and disposes of it in
    its finally section.

    --
    Lew
    Lew, Jul 10, 2009
    #6
  7. Lew wrote:
    > Lew wrote:
    >>> One way is to chain the I/O classes in separate try-catch blocks, so
    >>> that a later nullity check is unnecessary; you can use 'assert'
    >>> statements instead. The result is verbose but precise control over
    >>> what happens and how it's logged, and the safety of assertably
    >>> initialized streams and readers.
    >>>
    >>> public static void main( String [] args )
    >>> {
    >>> final InputStream is;
    >>> try
    >>> {
    >>> is = url.openStream();
    >>> }
    >>> catch ( IOException exc )
    >>> {
    >>> logger.error( "Cannot open URL stream. " + exc.getMessage() );
    >>> return;
    >>> }
    >>> assert is != null;
    >>>
    >>> final BufferedReader br;
    >>> try
    >>> {
    >>> br = new BufferedReader( new InputStreamReader( is ));
    >>> }
    >>> catch ( IOException exc )
    >>> {
    >>> logger.error( "Cannot open URL stream reader. " + exc.getMessage
    >>> () );
    >>> try
    >>> {
    >>> is.close();
    >>> }
    >>> catch ( IOException closex )
    >>> {
    >>> logger.error( "Cannot close URL stream. " + closex.getMessage
    >>> () );
    >>> }
    >>> return;
    >>> }
    >>> assert br != null;
    >>>
    >>> try
    >>> {
    >>> for ( String line; (line = br.readLine()) != null; )
    >>> {
    >>> System.out.println(line);
    >>> }
    >>> }
    >>> catch ( IOException exc )
    >>> {
    >>> logger.error( "Cannot read URL stream reader. " + exc.getMessage
    >>> () );
    >>> }
    >>> finally
    >>> {
    >>> try
    >>> {
    >>> br.close();
    >>> }
    >>> catch ( IOException closex )
    >>> {
    >>> logger.error( "Cannot close URL stream reader. " +
    >>> closex.getMessage() );
    >>> }
    >>> }
    >>> }

    >
    > Knute Johnson wrote:
    >> That's a pattern I've not seen before and I really like it, especially
    >> the for loop for reading the lines. The return in the catch block is
    >> pretty nifty too.

    >
    > The return in the catch block is necessary to ensure the asserted
    > invariant.
    >
    >> I am curious though if you really think that all those levels are
    >> really necessary? The only place that can throw an I/O exception is
    >> the URL.getInputStream() method and if it succeeds then the
    >> BufferedReader constructor will succeed. So closing the
    >> BufferedReader will suffice.

    >
    > I agree with you in this case. So the first try block would handle:
    >
    > final BufferedReader br;
    > try
    > {
    > br = new BufferedReader( new InputStreamReader( url.openStream() ));
    > }
    > catch ( IOException exc )
    > {
    > logger.error( "Cannot open URL stream reader. " + exc.getMessage() );
    > return;
    > }
    > assert br != null;
    >
    > The next try block would actually use 'br' and close it in its finally
    > block.
    >
    > The example I gave first was to address the OP's specific concern that
    > the reader assignments might fail separately from the stream retrieval,
    > and to illustrate the flexibility of the technique. Notice how easily
    > the idiom refactors to increase or decrease level of detail and
    > granularity of control by simply splitting out or merging try-catch
    > blocks. The key to the idiom is that one or more try-catch blocks
    > establish an invariant of a valid resource handle (the stream or reader
    > instance), and a separate try-finally block (with possible catch
    > clauses) uses that guaranteed resource and disposes of it in its finally
    > section.
    >


    Thanks Lew.

    --

    Knute Johnson
    email s/nospam/knute2009/

    --
    Posted via NewsDemon.com - Premium Uncensored Newsgroup Service
    ------->>>>>>http://www.NewsDemon.com<<<<<<------
    Unlimited Access, Anonymous Accounts, Uncensored Broadband Access
    Knute Johnson, Jul 11, 2009
    #7
    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. ZOCOR

    Closing IO Streams

    ZOCOR, Aug 27, 2004, in forum: Java
    Replies:
    3
    Views:
    511
    Michael Borgwardt
    Aug 27, 2004
  2. Replies:
    1
    Views:
    1,858
    Roedy Green
    Sep 17, 2005
  3. Knute Johnson

    Closing sockets and streams?

    Knute Johnson, Jan 21, 2006, in forum: Java
    Replies:
    14
    Views:
    647
    Thomas Hawtin
    Jan 24, 2006
  4. Evgeni Sergeev
    Replies:
    2
    Views:
    440
    Jean Brouwers
    Dec 28, 2004
  5. Juha Nieminen

    Re: Closing file streams

    Juha Nieminen, Jan 27, 2009, in forum: C++
    Replies:
    3
    Views:
    506
    James Kanze
    Jan 29, 2009
Loading...

Share This Page