Add central Cleaner instance

[smarsching]

Object.finalize() was deprecated in Java 9 and java.lang.ref.Cleaner was introduced as a better replacement for finalizers. I think that it would make sense to provide one central Cleaner instance in Phoebus, so that components that want to perform cleanup tasks when an object is garbage-collected, can do so.

The primary application are components that acquire external resources. Such components will typically implement Closeable, but they cannot ensure that all code that uses the component correctly calls close() under all circumstances. For such components, it makes sense to ensure that important cleanup tasks are at the latest carried out when the associated object is garbage-collected. In the past, this was done by overriding Object.finalize(), but now Cleaner provides a better alternative that is not deprecated.

As every instance of Cleaner is associated with a background thread, I think it makes sense to use one central instance in order to minimize the number of threads that are needed. So, I suggest adding a new class org.phoebus.framework.util.CleanerProvider to the core-framework module. This class would only have one public, static method called getCleaner() which returns the singleton cleaner instance.

I am happy to provide a PR that adds this class, but I would first like to discuss whether such a PR is desirable or whether there are better alternatives that I am overlooking. There might also be better suggestions for the name of the newly introduced class.

[kasemir]

Ideally, we should not need Object.finalize() nor java.lang.ref.Cleaner. Resources are simply garbage collected, or, if specific cleanup is necessary, we do that during controlled startup and shutdown, like when we open respectively close a display panel. The only point in the sources where I now find Object.finalize() is in the JFXRepresentation handling the media player. As with the previous SWT implementation, audio support continues to be a pain in the eyebrow, with the player simply doing nothing or hanging on some setups. So there's a hack of trying to handle the audio in a background thread without risking hangups and yes, that does include finalize. I would not take THAT as a starting point for adding general support for finalize or Cleaner, but rather look at reworking the audio support into using something like an on-close handler tied to the JFX window. What other cases do we have where we really see a need for finalize/Cleaner?

[smarsching]

I had this idea when working on the new JSON archive reader. I ported the code from an older version designed for the legacy version of CSS, and there I had overriden finalize in order to ensure that resources are freed when the ValueIterator is not closed explicitly. As finalizers are deprecated now, I replaced this code with a solution using a Cleaner.

In general, I like to practise “defensive programming”, so while I try ensure that resources are correctly released in my code, I do not want to rely on other code always doing so. BTW, the JDK code also employs this practice: For example, when you take a look at the code for FileInputStream, you will find that it ensures that the file descriptor is closed eventually, even if the input stream’s close method is never called. It does though through FileCleanable which employs a Cleaner that it retrieves from the JDK-internal class CleanerFactory.

So, I am suggesting that just like components in the JDK’s system modules have CleanerFactory, we should have a similar facility for Phoebus, so that components that employ defensive programming can ensure that their resources are cleaned up, even when they are used in an incorrect way.

Unfortunately, due to the lack of destructors and thus the RAII pattern in Java, accidentally forgetting to close a resource is easier than one might imagine. The try-with-resources statement has made it somewhat easier to deal with this, but it cannot be used in situations where access to a resource is needed beyond the scope of a single method.

For example, consider a simple class that might want to compare archived values for the last ten seconds with the samples from the equivalent period from yesterday:

public class ExampleResouce implements Closable {

  private ValueIterator iterator1;
  private ValueIterator iterator2;

  public ExampleResource(ArchiveReader reader) throws Exception {
    final var end1 = Instant.now();
    final var start1 = end.minusSeconds(10);
    this.iterator1 = reader.getRawValues("some-channel", start1, end1);
    final var end2 = end1.minusSeconds(86400);
    final var start2 = end2.minusSeconds(10);
    this.iterator2 = reader.getRawValues("some-channel", start2, end2);
  }

  // Methods that do something with the iterators would go here.

  @Override
  public void close() throws IOException {
    if (this.iterator1 != null) {
      iterator1.close();
      iterator1 = null;
    }
    if (this.iterator2 != null) {
      iterator2.close();
      iterator2 = null;
    }
  }
}

At a first glance, it looks like this code does proper resource management (assuming that the code using ExampleResource always calls close()). But when taking a closer look, there is a possible resource leak when the second call to getRawValues(…) throws an exception. In this case, iterator1 is never closed.

I agree that “good” code will handle this correctly by catching exceptions in the constructor and releasing resources that have already been initialized at this point. However, for code that is more complex than the example above, it might not always be obvious that something like this can happen. Therefore, I think that releasing a resource when the garbage collector has collected the object owning it (and the resources have not been released explicitly yet), is a good practice. The JDK developers seem to agree with me. 🙂

Please note that I do not recommend relying on the garbage collector for resource management. Due to the non-deterministic nature of garbage collection, closing resources explicitly is important. So, using a Cleaner to release the resources owned by an object when this object is garbage-collected is not an excuse for shoddy resource management. It rather is a last line of defense when, despite all efforts to properly manage resources, close() is not called due to bugs that might be hard to find.

[kasemir]

If I understand correctly, you'd like to add a cleaner not to generally rely on that for resource cleanup, but as a fallback that will usually look like this:

   logger.log(Level.WARNING, "Resource XYZ was not closed");
   xyz.close();

Yes, that might help find resource leaks, especially small ones that can go unnoticed for a long time.

[smarsching]

Yes, this is exactly what I want to do. So far, I hadn’t considered writing a warning message to the log, but this is a good idea.

So, a ValueIterator employing this scheme and referencing an InputStream could look like this:

public class ExampleValueIterator implements ValueIterator {

    // We use a static class instead of a lambda expression because the cleanup action must not retain a reference to the original object. Otherwise, that object could never be garbage collected.
    private static class CleanupAction implements Runnable {

        private volatile boolean closed;
        private final InputStream input_stream;
        private final Logger logger;

        private CleanupAction(final InputStream input_stream) {
            this.closed = false;
            this.input_stream = input_stream;
            this.logger = Logger.getLogger(ExampleValueIterator.class.getName());
        }

        @Override
        public void run() {
            // This method is only called once, even if cleanable.clean() is
            // called multiple times.
            try {
                this.input_stream.close();
            } catch (IOException e) {
                logger.log(Level.SEVERE, "Could not close InputStream.", e);
            }
            if (!closed) {
                logger.warning("Instance of ExampleValueIterator was never closed.");
            }
        }
    }

    private final Cleaner.Cleanable cleanable;
    private final CleanupAction cleanup_action;
    private InputStream input_stream;

    public ExampleValueIterator(InputStream input_stream) {
        this.cleanup_action = new CleanupAction(input_stream);
        final var cleaner = CleanerProvider.getCleaner();
        this.cleanable = cleaner.register(this, this.cleanup_action);
        this.input_stream = input_stream;
    }

    // hasNext() and next() omitted for clarity.

    @Override
    public void close() throws IOException {
        if (cleanup_action.closed) {
            return;
        }
        cleanup_action.closed = true;
        cleanable.clean();
    }

}

The CleanupAction could of course contain additional information for logging (e.g. an instance identifier, etc.).

I am still a bit unsure whether the utility class should be called CleanerProvider or CleanerFactory. On one hand, it is not a factory in the original sense, but on the other hand it might better fit into the overall naming scheme.

[kasemir]

Call her Klementine , the Ariel cleanup expert. https://www.planet-wissen.de/gesellschaft/sauberkeit/waesche_waschen/pwieklementinediewaschfraudernation100.html

[smarsching]

🤣🤣🤣