[JENKINS-54566] finalize vs. flush

[jglick]

JENKINS-54566

Amends #81. jenkinsci/remoting#308 suppresses the error, but this should be the actual fix. Note that flushing a soon-to-be-collected stream is intended as a best effort to capture any stray messages not already delivered, due perhaps to callables neglecting to explicitly flush.

[dwnusbaum]

Looks good to me. It would be nice to add a few basic unit tests for Buffer, but I don't consider that a blocker.

[svanoort]

Seriously hacky, but I think this will work IIUC what it's doing.

Makes me wonder why Remoting doesn't have a way to register a callback to execute before disposing the lease on an object.

[svanoort]

Weak +1 -- not sure I could predict the possible failure modes of this approach.

I would really like to see more tests on this, but don't see any obvious bugs.

[svanoort]

Also the fact that we're not doing array-at-a-time writes feels like it'll turn up as a bottleneck in the future.

stale

[svanoort]

I actually really like this approach -- it separates concerns nicely

[dwnusbaum]

@jglick Do you remember if there are any tests that specifically exercise GCFlushedOutputStream? Calling PhantomReference.enqueue here ourselves (called in turn by the GCFlushedOutputStream constructor) causes the cleanup code to run the next time the timer task executes (at most ~10 seconds later) regardless of the reachability of the GCFlushedOutputStream and then never again, so I am not sure if this class is doing anything meaningful right now.

You can test the behavior with a class like this.

public class Test {
    private static class Ref extends PhantomReference<Object> {
        static final ReferenceQueue<Object> queue = new ReferenceQueue<>();
        
        private final Runnable cleanup;

        public Ref(Object referent, Runnable cleanup) {
            super(referent, queue);
            this.cleanup = cleanup;
        }
    }

    public static void main(String[] args) throws Exception {
        Object o = new Object();
        var ref = new Ref(o, () -> {
            System.out.println("Cleaned up");
        });
        // Uncomment the next line to see what happens when `PhantomReference.enqueue` is called manually.
        // System.out.println(ref.enqueue());
        System.out.println("GC 1");
        System.gc();
        while (Ref.queue.poll() instanceof Ref ref2) {
            System.out.println("Running cleanup up while reference still exists!");
            ref2.cleanup.run();
        }
        Thread.sleep(2 * 1000);
        System.out.println(o); // Make sure o cannot be GC'd prior to this point.
        o = null;
        System.out.println("GC 2");
        System.gc();
        Thread.sleep(2 * 1000);
        while (Ref.queue.poll() instanceof Ref ref2) {
            System.out.println("Running cleanup at correct time.");
            ref2.cleanup.run();
        }
    }
}

Alternatively, you can add logging and Thread.sleep calls in FlushRef.register to observe the cleanup code running within the extent of the GCFlushedOutputStream constructor, when the object must still be strongly reachable.

I think we can make the class work as intended by switching to Cleaner. See #388. Trying to fix the issue directly by dropping the explicit call to enqueue runs into the problem that we need something to hold a reference to the FlushRef that outlives the GCFlushedOutputStream, otherwise the FlushRef itself will just get GC'd and the cleanup code will never run.

We could also just delete the class, given this has apparently never worked as intended. See #387.

Any thoughts?

[jglick]

Sorry, I recall little of this. It is possible tests in pipeline-cloudwatch-logs implicitly exercise this behavior. There are also some JEP-210-related tests in workflow-durable-task-step which deal with remote TaskListenerDecorators or something like that.