Closing a Java Scanner object made during constructor chaining

Question

I'm using constructor chaining, and I'm worried that it's causing a resource leak. Here are my two constructors:

/**
 * Constructor to build the map based off of a file. Redirects to the Scanner-based constructor
 * @param fileName the name of the file to open
 */
public GeoMap(String fileName) throws FileNotFoundException {
    this(new Scanner(new File(fileName)));
}

/**
 * Constructor to build the map based off of a Scanner. (Probably from an open file.)
 * @param scanner the Scanner to read
 */
public GeoMap(Scanner scanner) {
    // goes on to read the string data and make an object...

It's important that the object be created from any type of Scanner (keyboard, file, etc.), though it'll usually be from a file. The problem is that I think there's a resource leak going on here. Whenever I'm reading a file, I like to close it when I'm done. Problem is, the constructor chaining means that the this() call must be the first line. I'd be inclined to do something like this:

this(Scanner scannerToClose = new Scanner(new File(fileName)));

In my mind that would give me the name of a Scanner I could then close out. But that seems to really confuse the compiler--I get about 5 compile-time errors out of it, including a lot of "cannot find symbol" problems that imply that the compiler's just not wired for this sort of thing. Does Java support this? Or do I need to make a totally different initFromScanner() function that both constructors call? (Not elegant.)

Thanks.

Answer 1

Call scanner.close() at the end of your GeoMap(Scanner scanner) constructor.

This will close the Scanner created in GeoMap(String filename) since a reference to it is passed into the GeoMap(Scanner scanner) as scanner.

In essence, the scanner variable points to the new scanner that was created, so calling scanner.close() anywhere, in any method, closes it for any and all other methods it may be in the scope of.

Here is a program which demonstrates the object oriented nature of Scanners:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.Scanner;
public class Main
{
    static class Test
    {
        String name;
        public Test(String filename) throws FileNotFoundException
        {
            this(new Scanner(new File(filename)));
        }
        public Test(Scanner scanner)
        {
            name = scanner.nextLine();//body of constructor
            scanner.close();
            System.out.println("Your name is "+ name);
            scanner.close();
            
            /*These next lines of code show that the Scanner is closed */
            String throwsException = scanner.nextLine();
            System.out.println(throwsException + "here");//unreachable
        }
    }
    public static void main(String[] args)
    {
        try
        {
            Test temp = new Test("input.txt");
        }
        catch(Exception e)
        {
            System.out.println(e);
        }

    }
    

}

input.txt: Smitty

output:

Your name is Smitty
java.lang.IllegalStateException: Scanner closed

In essence it doesn't matter where the Scanner is created, if it is closed at any point, it is closed everywhere that it is in scope.

Answer 2

I assume that your issue is that you only want to close the involved scanner if you have created it in your constructor that takes fileName. I don't think there's anything wrong with your idea of having an init method that both of your constructors call. I don't think that's inelegant.

I think what I would do is create a third private constructor instead of an init method. It's really the same thing either way, although maybe at some point you'd want to be able to pass in a pre-built Scanner that you want closed at the end of the constructor call, in which case you could make this new constructor public so you could call it from the outside.

In either case, what I'd do is pass a boolean "closeScanner" parameter to the new constructor/method that indicates if the scanner should be closed or not. Here's my idea in code:

import java.io.File;
import java.io.FileNotFoundException;
import java.util.Scanner;

class GeoMap {

    /**
     * Constructor to build the map based off of a file. Redirects to the Scanner-based constructor
     *
     * @param fileName the name of the file to open
     */
    public GeoMap(String fileName) throws FileNotFoundException {
        this(new Scanner(new File(fileName)), true);
    }

    /**
     * Constructor to build the map based off of a Scanner. (Probably from an open file.)
     *
     * @param scanner the Scanner to read
     */
    public GeoMap(Scanner scanner) {
        this(scanner, false);
    }

    private GeoMap(Scanner scanner, boolean closeScanner) {
        // goes on to read the string data and make an object...
        if (closeScanner)
            scanner.close();
    }
}

Answer 3

First, your class GeoMap should define how it handles a scanner that is given to it in the constructor; usually, when it is allowed to create its own Scanner instances as in your sample, the policy is that the GeoMap instance can do whatever it wants with that scanner, including closing it – this means it owns it, and ownership is transferred in the respective constructor.

If this is not the case (it does not own the scanner), you either have to drop the GeoMap(String) constructor (because, when not the GeoMap instance owns it, who else do and takes care of it later?), or you have to come to a design similar to that below:

class GeoMap 
{
    private final Scanner m_Scanner;
    private final boolean m_MayCloseScanner;

    /**
     *  Creates a GeoMap from a file.
     *
     *  @param fileName The name of the file to open.
     */
    public GeoMap( String fileName ) throws FileNotFoundException 
    {
        this( new Scanner( new File( fileName ) ), true );
    }  // GeoMap()

    /**
     *  Creates a GeoMap from a Scanner instance.
     *
     *  @param scanner The Scanner to read
     */
    public GeoMap( Scanner scanner ) 
    {
        this( scanner, false );
    }  // GeoMap()

    /**
     *  Internal constructor.
     *  
     *  @param scanner The scanner to read.
     *  @param mayClose true, if this instance of GeoMap may close the
     *      given Scanner instance, false otherwise.
     */
    private GeoMap( Scanner scanner, boolean mayClose ) 
    {
        m_Scanner = scanner;
        m_MayCloseScanner = mayClose;
    }  // GeoMap()

    …
}
// class GeoMap

Here the ownership is tracked by the flag m_MayCloseScanner. Unfortunately, this does not yet solve your issue with the resource leak: the scanner will still not be closed, when the GeoMap instance is no longer used.

When your GeoMap instance will not own the scanner at all, you don't care, the resources taken by the scanner are a POOP (a problem of other people).

Ok, when you will need the scanner only for the initialisation of the GeoMap instance, you can have an init() method that closes the scanner when done:

…
public void init()
{
    // Do something with the scanner ...
    …

    // Close the scanner when done.
    m_Scanner.close()
}  // init()
…

Of course, when GeoMap may or may not own the scanner, the closing line needs to look like this: if( m_MayCloseScanner ) m_Scanner.close;.

But if that init option does not work, you need a destructor for GeoMap instances. The concept of a destructor does not exist in Java, the closest to it was to implement finalize(), but this was deprecated some time ago (finally with Java 9), with good reason.

Have a look to this post on how to use Cleaner, PhantomReference and Closeable for your GeoMap class. It looks a bit confusing in the beginning, but reveals to be relatively straight forward in the end.

Answer 4

Lets start with this:

public GeoMap(Scanner scanner) {
    ...
}

Is there a resource leak here? Well, it depends on where the responsibility for closing the Scanner lies.

• If responsibility lies in the constructor, then we can plug the leak like this:

  public GeoMap(Scanner scanner) {
      try (Scanner s = scanner) {
          // original body
      }
  }
  

  This is the ideal solution, but it assumes that the effect lifetime of the Scanner is the constructor.

• If it is the caller responsibility, then the caller needs to deal with leak prevention. That is doable ... but outside of the scope of this question.

• If it is neither the constructor or the caller's responsibility, then you need to treat the GeoMap itself as a resource, with all that that entails.

Now we consider this:

public GeoMap(String fileName) throws FileNotFoundException {
    this(new Scanner(new File(fileName)));
}

Firstly, does new Scanner(new File(fileName)) present a potential resource leak?

In theory, yes. The Scanner constructor could open a stream for the file, and then fail, leaving the stream open.

In practice, it is highly unlikely. If we ignore bugs in the class library, and application bugs like using unrecognized character set names, the only cause of a spontaneous failure of new Scanner that could leak a file descriptor, etc is if you got an OOME. But that is likely to trigger a full GC anyway.

So what happens after that?

The answer depends on the earlier answer of where the responsibility lies in the GeoMap(Scanner) constructor.

• If the responsibility lies in that constructor, we know how to avoid the leak; see above.

• Otherwise ... we have problem:

  • There are possible solutions, but they may involve changing the way that the Scanner is used.
  • There may also be leaks involving the use of the that constructor directly.

---

In summary, depending on how you specify and implement GeoMap(Scanner), the GeoMap(String) constructor can be implemented to be leak proof in practice.