typetools / typetools/checker-framework

Resource Leak Checker: multiple false positives trying to build a collection of owned resources

Open
#5,969 3 comments 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

ResourceLeakChecker
Dominant language
Java
Stars
1.1k
Forks
440
Avg merge
1d 12h
Merged PRs (30d)
134

Description

I am working on a program that passes around "bundles" of resources. When a client is done with a bundle, all of the underlying resources have to be closed.

Today my program represents bundles as List<Resource>. I more or less understand why List<@Owned Resource> is disallowed, so I set out to write a small class that can own more than one resource. I was not able to make headway using arrays, but I almost succeeded using a linked data structure.

However, the Resource Leak Checker still issues a number of errors that I am unable to fix. In particular, I would like it to be powerful enough to verify the loop in disconnectAll():

import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods;
import org.checkerframework.checker.mustcall.qual.CreatesMustCallFor;
import org.checkerframework.checker.mustcall.qual.MustCall;
import org.checkerframework.checker.mustcall.qual.Owning;
import org.checkerframework.checker.nullness.qual.Nullable;

import java.io.Closeable;
import java.io.IOException;

@MustCall("disconnectAll")
public class ResourceBundle {

    private @Owning @Nullable Node head = null;

    @CreatesMustCallFor("this")
    public boolean add(@Owning Closeable resource) {
        head = new Node(resource, head);
        return true;
    }

    @EnsuresCalledMethods(value="this.head", methods="disconnect")
    public void disconnectAll() {
        Node ptr = head;
        while (ptr != null) {
            ptr = ptr.disconnect();
        }
    }

    @MustCall("disconnect")
    private static class Node {
        @Owning private final Closeable resource;
        @Owning private final @Nullable Node next;

        public Node(@Owning Closeable resource, @Owning @Nullable Node next) {
            this.resource = resource;
            this.next = next;
        }

        @EnsuresCalledMethods(value="this.resource", methods="quietDisconnect")
        @EnsuresCalledMethods(value="this.next", methods="disconnect")
        public @Owning @Nullable Node disconnect() {
            try {
                resource.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
            return next;
        }
    }

}

Checker Framework version 3.34.0 issues four spurious-looking errors.

  1. At head = new Node(resource, head);: "error: [required.method.not.called] @MustCall method disconnect may not have been invoked on field head or any of its aliases [...] Non-final owning field might be overwritten". I believe the error is incorrect; the Node constructor takes ownership of the old value of head.
  2. At public void disconnectAll(): "error: [contracts.postcondition] postcondition of disconnectAll is not satisfied. [...] found : no information about this.head [...] required : this.head is @CalledMethods("disconnect")". This message is extremely confusing. There must be a way to write this loop that satisfies the RLC, but I can't figure it out.
  3. At @Owning private final Closeable resource;: "@EnsuresCalledMethods written on MustCall methods doesn't contain method close". Likely due to #5911.
  4. At @Owning private final @Nullable Node next;: "@EnsuresCalledMethods written on MustCall methods doesn't contain method disconnect". Likely due to #5911.

I was able to make a little progress using a silly-looking helper definition vaguely inspired by C++ std::move:

    @Pure
    private static @Owning @Nullable Node move(@Owning @Nullable Node x) {
        return x;
    }

With that definition, error (1) can be silenced using this code instead:

        Node oldHead = move(head);
        head = new Node(resource, oldHead);

However, I still have no way to silence error (2). I had thought that changing the first line of disconnectAll to Node ptr = move(head); might do the trick, but that seems to have no effect.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start by reproducing the four Resource Leak Checker diagnostics from the ResourceBundle example with Checker Framework 3.34.0, focusing on add(), disconnectAll(), Node.resource, and Node.next. Read the ownership and @EnsuresCalledMethods behavior around these entry points. Done means the reported false positives are addressed and the disconnectAll() loop can be verified without the helper workaround.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
devtools
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.