Feature - Implemented Reducing number of warnings: Resource leak, "checkpoint" is never closed

[fredg1]

This patch suppresses 29 "Resource leak, "checkpoint" is never closed" warnings.

Here is what they mean, and why they can be ignored:

All 29 cases of this warning follow this pattern:

Checkpoint checkpoint = new Checkpoint();
try
{
    // do something
}
finally
{
    checkpoint.restore();
}

Checkpoint is a class coming from SpecialOutfit.java, that implements the interface AutoCloseable.
AutoCloseable is used to defined an object that holds resources, just like Closeable. Compilers will throw a warning if they notice that instances of such class fail to call their close() method one way or another.

Here are the resource-related methods of Checkpoint:

public class SpecialOutfit
{
    private static final Set<Checkpoint> allCheckpoints = new HashSet<Checkpoint>();

    public static class Checkpoint
        implements AutoCloseable
    {
        public Checkpoint()
        {
            // <saves your current equipment>

            synchronized ( SpecialOutfit.class )
            {
                SpecialOutfit.allCheckpoints.add( this);
            }
        }

        public void restore()
        {
            // If this checkpoint has been closed, don't restore using it
            if ( !this.known() )
            {
                return;
            }

            // <re-equips the content of the Checkpoint>

            this.close();
        }

        public void close()
        {
            // For use with "try with resource", once we upgrade to a better Java version
            synchronized ( SpecialOutfit.class )
            {
                SpecialOutfit.allCheckpoints.remove( this );
            }
        }

        public boolean known()
        {
            // For use with "try with resource", once we upgrade to a better Java version
            synchronized ( SpecialOutfit.class )
            {
                return SpecialOutfit.allCheckpoints.contains( this );
            }
        }
    }
}

Compilers are unable to understand that the scenario inside Checkpoint.restore(), in which Checkpoint.close() isn't called, simply happens because its result was already achieved. Hence, the warning will be thrown, without being an actual problem.

Edit: spelling

 

[fredg1]

Note: for those who wonder what's the difference between Closeable and AutoCloseable, as well as those who noticed the comments talking about something called "try with resource", allow me to explain further.

AutoCloseable is an interface that can use "try with resource" (something that was not available in the java version when this class was made).

"Try with resource" is a try statement that automatically calls class.close() at the end, done like so:

try ( Checkpoint checkpoint = new Checkpoint() )
{
    // do stuff
}

That's it. When exiting the try, checkpoint.close() would be called.

In its current state, though, this is not good. We want checkpoint.restore() to be called, not close().


A way to make this possible would be to change these methods. For example

public void restore()
{
    // <re-equips the content of the Checkpoint>
}

public void close()
{
    // If this checkpoint has been closed already, don't restore using it
    if ( !this.known() )
    {
        return;
    }

    this.restore();

    this.remove();
}

public void remove()
{
    synchronized ( SpecialOutfit.class )
    {
        SpecialOutfit.allCheckpoints.remove( this );
    }
}

would be a possible alternative allowing "try with resource"s to be used (every current call of Checkpoint.restore() would be replaced by a call of Checkpoint.close(), and every call of Checkpoint.close() would be replaced by a call of Checkpoint.remove() )

However, not having a very good knowledge of the workings of Checkpoints, I'll leave it to someone more experienced to make this call.

 

[gausie]

Can you give an example of how I can recreate the warning?

Your intuition in your second post is correct, we want to flip the calling order such we implement .close() to handle the restore function.

 

[fredg1]

without the @suppressWarning, your IDE should automatically mark the checkpoint at its declaration. You probably have that type of warning disabled

 

[gausie]

I took your basic concept and made r20695 with it

 

[MCroft]

Do we always want to restore the prior checkpoint state? That was my concern when Fred and I were looking at this.

 

[fredg1]

No. There are 2-3 instances where the restoration was only happening on a condition. However, Gausie included it in the patch, making a new Checkpoint constructor that takes whether the checkpoint is to be restored at the end or not.

 

[MCroft]

public class SpecialOutfit
{
    ...
            // For use with "try with resource", once we upgrade to a better Java version
            synchronized ( SpecialOutfit.class )
    ...
}

This was my favorite comment in that whole class file.

But now I wonder if there are other similar comments about "change this when we upgrade to Java 1.8".

 

[Veracity]

Yeah, I did that. I implemented the Checkpoint class. And I was inspired by "try with resource" which I discovered when I went to work at Amazon (on Alexa) for two years, where we used Java 8 and I learned lambdas and streams and so much more about modern Java than we had in KoLmafia with Java 6. I am retired now.

But, look at the Cargo Shorts database. Lambdas and Streams.

I am very happy we are at Java 8, now. Even though I no longer have a working JDK on my new computer and can't even BUILD from the sources, much less contribute fixes.