Mark a method as returning an existing closeable to avoid spurious warning at usage site

Question

Given a class MyClass using an internal closeable object, myCloseable and providing a method getCloseable() that returns it; Eclipse, if configured for this kind of closeable resource warnings, will systematically warn each time anyone calls getCloseable(), saying that the closeable “may not be closed at this location”.

It is nice to have such warnings about resources that should be closed in case one forgets to close them, so I like to leave this check enabled. But in the just described scenario, it is quite annoying. In principle, it seems possible to mark the getCloseable() method with an annotation, say, @existingCloseable, telling the compiler that “it’s okay, I am just returning a resource that already existed, not creating a new one, thus the caller is not supposed to close it”.

Has such annotation been considered for adoption, by Eclipse or other IDEs? I couldn’t find discussions about it, so I suspect it has not. I wonder why: the pattern described in my example seems quite common and natural to me. Would the solution I consider with annotations not work, or have drawbacks I didn’t think of?

Example

Here is a (silly) example where the closeable is an OutputStream. The method fromPath produces a warning (if not suppressed) about s being not closed, and I do not mind about this: this warning seems adequate and need to be suppressed only once. The annoying part, and the object of my question, is the warning that appears from the main method: “Potential resource leak: 'target' may not be closed”. This warning will appear each time any user of my class will use getTarget. I would like to disable it once and for all by annotating the method getTarget in order to let the compiler know that this method returns a resource that the caller is not supposed to close. To the best of my knowledge, this is currently unsupported in Eclipse, and I wonder why.

public class MyWriter implements AutoCloseable {
    public static void main(String[] args) throws Exception {
        try (MyWriter w = MyWriter.fromPath(Path.of("out.txt"))) {
            // …
            OutputStream target = w.getTarget();
            target.flush();
            // …
        }

    }

    @SuppressWarnings("resource")
    public static MyWriter fromPath(Path target) throws IOException {
        OutputStream s = Files.newOutputStream(target);
        return new MyWriter(s);
    }

    private OutputStream target;

    public OutputStream getTarget() {
        return target;
    }

    private MyWriter(OutputStream target) {
        this.target = target;
    }

    @Override
    public void close() throws Exception {
        target.close();
    }
}

Discussion

I have edited my question, which originally asked whether my code can be modified to avoid the warning: I realized this is not really the question I am interested in, I am rather interested about the annotation based solution I thought about – sorry for this.

I realize this example is silly: with no further context, and as several answers correctly point out, it looks like I should rather wrap all methods of the stream and never return the stream to the outside world. Unfortunately, it is difficult to make this example realistic while also keeping it small.

One example is well known, however, so that I do not need to give it in detailed form here: in a servlet, one can call getOutputStream(), and this is a typical case that illustrates my point: the method getOutputStream() returns a stream that the caller does not need to close, but Eclipse will warn about a potential resource leak each and every time it is called (that is, in every servlet), which is a pain. It is also clear why the conceptors of this method did not simply wrap everything instead of returning the stream itself: it is useful to get an object that implements this well known interface, because it will then be possible to use it with other libraries and methods that expect to interact with a stream.

As another illustration of my point, imagine that the getCloseable() method is used only internally, thus the method is package-visible instead of being public. The implementation of getCloseable() might be complex, with inheritance playing a role, for example, so that it is not necessarily possible to replace this call with a simple access to the underlying field as in my small example. In such a case, it feels like not KISS at all to implement a whole wrapper instead of returning an already existing interface, just to keep the compiler happy.

Answer 1

Potential resource leak problem

You get here a Potential resource leak warning, which is disabled by default, not a regular Resource leak warning which is enabled by default. In contrast to a Resource leak warning, a Potential resource leak warning will be shown, if you do not create a AutoCloseable yourself, but get it from somewhere and do not close it, neither explicitly by calling close() nor implicitly by using try-with-resource.

Whether the resource is closed somewhere you got it from, as in your case, might be computed for simple cases, but not for all cases. It is the same problem as the halting problem. For example, the creation of the AutoCloseable or the closing of it might be delegated to somewhere where it is delegated again, and so on, and if there is an if, a switch, or a loop, all branches must be followed to be sure.

Even in your simple-looking example, it's not that simple. By making the only one constructor private, you prevent that the class can be extended, otherwise it would be possible to override close() without calling super.close() causing target.close() never being called. But since private OutputStream target; is not final, there can still be a real resource leak as in the following:

public static void main(String[] args) throws Exception {
    try (MyWriter w = MyWriter.toPath(Path.of("out.txt"))) {
        // …
        OutputStream target = w.getTarget();
        w.target = new FileOutputStream("out2.txt");
        // …
    }
}

So the compiler would have to check besides the class cannot be overridden, if the field holding the inner AutoCloseable is either final or privat and effectively final (set only when initializing) and that the inner AutoCloseable will be closed in the close() of the outer AutoCloseable.

To summarize, if you don't generate the resource yourself but get it from somewhere, there is no guarantee that the compiler can compute in limited time whether it will be closed (see halting problem). For those cases there is the Potential resource leak warning, in addition to the Resource leak warning, which can be deactivated separately and is deactivated by default.

Annotation based solution?

The @SuppressWarnings annotation is for suppressing the named compiler warnings in the annotated element. But in this case here an annotation would be required that tells the compiler that the returned AutoCloseable does not need to be closed, similar to the @SafeVarargs annotation or to null annotations. To my knowledge, such an annotation does not yet exist, at least not in the system library. In order to use such an annotation, one would first have to add a dependency containing this annotation to the project (exactly the same situation as it is for null annotations, which unfortunately are still not part of the system library, maybe because javac does currently not support annotation based null analysis in contrast to the Eclipse compiler and other IDEs and tools).

There would also need to be a solution for cases like the following, where it is determined by an input parameter whether the returned value needs to be closed or not (maybe via another annotation):

public static BufferedOutputStream toBufferedOutputStream(OutputStream outputStream) {
    return new BufferedOutputStream(outputStream);
}

So first, such annotations would have to exist (best not Eclipse-specific and as part of the system library), in order Eclipse can support them.

Solution for the given example

As solution, do not expose target by e.g. wrapping it and expanding from OutputStream:

public class MyWriter extends OutputStream {
    public static void main(String[] args) throws Exception {
        try (MyWriter w = MyWriter.of(Path.of("out.txt"))) {
            // …
            w.flush();
            // …
        }
    }

    @SuppressWarnings("resource")
    public static MyWriter of(Path target) throws IOException {
        OutputStream s = Files.newOutputStream(target);
        return new MyWriter(s);
    }

    private final OutputStream target;

    private MyWriter(OutputStream target) {
        this.target = target;
    }

    @Override
    public void close() throws IOException {
        target.close();
    }

    @Override
    public void write(int b) throws IOException {
        target.write(b);
    }
    
    @Override
    public void flush() throws IOException {
        target.flush();
    }
}

Or even better:

public class MyWriter {
    
    public static void main(String[] args) throws Exception {
        MyWriter.writeToFile(Path.of("out.txt"), w -> {
            try {
                // …
                w.flush();
                // …
            } catch (IOException e) {
                // error handling
            }
        });
    }

    public static void writeToFile(Path target, Consumer<OutputStream> writer) throws IOException {
        try (OutputStream out = Files.newOutputStream(target);) {
           writer.accept(out);
        }
    }

}