Bob, It would seem to me all the instant variables used in the rollback and/or the methods it references would have to be sent arguments to the Runnable and the same actions rollback does and the methods it calls would have to be done by the Runnable. Is that correct?
On Wed, Mar 19, 2025 at 4:57 PM Dan S <dsti...@gmail.com> wrote: > Bob thanks for your response. I am still confused how to apply the example > you have to the actual StandardProcessSession as ultimately its rollback > method has to be called and this method is not static. So how is it > possible for the Runnable registered with the Cleaner not to have a handle > on the StandardProcessSession instance? > > On Tue, Mar 18, 2025 at 10:46 AM Bob Paulin <b...@apache.org> wrote: > >> Formatting came through poorly. Please see [1] >> >> - Bob >> >> [1] https://gist.github.com/bobpaulin/ce6a83e45cd6bbf34051935324f10c63 >> >> On 2025/03/18 14:40:19 Bob Paulin wrote: >> > Hi >> > >> > In both cases it appears that finalize is being used as resource clean >> > up following the object being de-referenced. In both cases finalize is >> > not the primary means of cleanup but rather a fall-through for >> > exceptional cases where processing is interrupted. The consequences >> are >> > different in each case in the case of AbstractCacheServer the >> > cacheServer resource is not released which is a memory concern. In the >> > case of StandardProcessSession it's a call to rollback which is >> > important for data integrity. So I'm not sure removal is without >> > consequences but more so for StandardProcessSession. I also agree that >> > implementing closeable might not be possible in all cases for >> > StandardProcessSession. >> > >> > Cleaner looks like the simplest approach given it's wrapping some of >> the >> > complexity in working with the ReferenceQueue[1]. There are more >> > sophisticated things we could do using the ReferenceQueue directly (see >> > [2]). There are some important caveats to implementing this that would >> > need to be followed to ensure it works. First we can't use references >> > to the object AbstractCacheServer or StandardProcessSession within the >> > runnable or it will not trigger (since that would create a reference >> > thus it would not be Phantom). It may be valuable to create a static >> > method on each of these classes that requires the constructor objects >> to >> > be passed to it in order to discourage the usage references to the >> > object. Second we'll want to avoid passing "this.varaibles" to the >> > static method. Below is some example code for reference. Notice what >> > happens if you try to pass this.value instead of value. The cleaner is >> > not called because it's not a phantom ref anymore. So I'm not sure it's >> > required to pass the cleaning function to an additional class (as >> > demonstrated below) but it might help keep things cleaner and further >> > avoid references to the object that we're cleaning from. Hope this is >> > helpful. >> > >> > packagecom.bobpaulin.nifi; >> > >> > importjava.lang.ref.Cleaner; >> > >> > importjava.lang.ref.PhantomReference; >> > >> > importjava.lang.ref.ReferenceQueue; >> > >> > importorg.slf4j.Logger; >> > >> > importorg.slf4j.LoggerFactory; >> > >> > publicclassCleanerExample { >> > >> > privatestaticfinalLogger LOGGER= >> > LoggerFactory.getLogger(CleanerExample.class); >> > >> > //Static class just for demo purposes >> > >> > publicstaticclassStandardProcessorSession { >> > >> > privatestaticfinalCleaner cleaner= Cleaner.create(); >> > >> > privatefinalCleaner.Cleanable cleanable; >> > >> > privatefinalString value; >> > >> > publicStandardProcessorSession(finalString value) { >> > >> > this.value= value; >> > >> > finalRunnable cleanUpRunner= () -> rollback(value); >> > >> > this.cleanable= cleaner.register(this, cleanUpRunner); >> > >> > } >> > >> > publicstaticvoidrollback(finalString value) { >> > >> > System.out.println("Rollback Called. Value is "+ value); >> > >> > } >> > >> > } >> > >> > publicstaticvoidmain(finalString[] args) { >> > >> > StandardProcessorSession processSession= >> > newStandardProcessorSession("Hello"); >> > >> > //Needed for testing >> > >> > finalReferenceQueue<StandardProcessorSession> refQueue= >> > newReferenceQueue<>(); >> > >> > // Create a phantom reference artificially >> > >> > finalPhantomReference<StandardProcessorSession> helloPhantomReference= >> > newPhantomReference<>( >> > >> > processSession, refQueue); >> > >> > newThread(() -> { >> > >> > LOGGER.info("Starting ReferenceQueue consumer thread."); >> > >> > booleanreferenceNotFound= true; >> > >> > while(referenceNotFound) { >> > >> > try{ >> > >> > @SuppressWarnings("unchecked") >> > >> > finalPhantomReference<StandardProcessorSession> reference= >> > (PhantomReference<StandardProcessorSession>) refQueue >> > >> > .remove(); >> > >> > referenceNotFound= false; >> > >> > LOGGER.info("Finalized has been finalized."); >> > >> > } catch(finalInterruptedException e) { >> > >> > // Just for the purpose of this example. >> > >> > break; >> > >> > } >> > >> > } >> > >> > LOGGER.info("Finished ReferenceQueue consumer thread."); >> > >> > }).start(); >> > >> > // Lose the strong reference to the object. >> > >> > processSession= null; >> > >> > // Attempt to trigger a GC. >> > >> > System.gc(); >> > >> > } >> > >> > } >> > >> > >> > - Bob >> > [1] https://inside.java/2022/05/25/clean-cleaner/ >> > [2] >> > >> https://unitstep.net/blog/2018/03/10/java-phantomreferences-a-better-choice-than-finalize/ >> > >> > On 3/17/2025 4:11 PM, Dan S wrote: >> > > I am currently working on NIFI-14340 >> > > <https://issues.apache.org/jira/browse/NIFI-14340> trying to replace >> the >> > > overridden methods finalize in both AbstractCacheServer and >> > > StandardProcessSession as >> > > per the javadocs >> > > < >> https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/lang/Object.html#finalize() >> > >> > > finalize >> > > is deprecated for removal. >> > > >> > > So my first question is whether we would like a replacement for these >> > > finalize methods or is the solution for this ticket is as simple as >> just >> > > removing them? >> > > If we would like to replace them based on the suggestions given in >> JEP 421 >> > > <https://openjdk.org/jeps/421> then I have a few other questions. >> > > The first suggested solution is to make the class implement >> > > java.lang.AutoCloseable and then instantiate and use it in a try with >> > > resources statement within a single lexical scope. >> > > It does not seem to me that is plausible for AbstractCacheServer >> which is a >> > > controller service and from what I understand is not used within a >> single >> > > lexical scope. >> > > It also does not seem this would work for StandardProcessSession as >> there >> > > are cases where instances of ProcessSession handle transferring flow >> > > file(s) and commits in a catch block. If used >> > > in a try resources it would be closed before the catch block code is >> called. >> > > The second suggestion is to use Cleaners but that also seems >> difficult as >> > > it seems from what I have read the right way to implement this would >> be >> > > passing AbstractCacheServer and StandardProcessSession into a >> different >> > > class to handle cleaning. I am not sure how that would be done for >> these >> > > classes. >> > > >> > > Can I please get guidance on how we should proceed regarding these >> finalize >> > > methods? Thank you! >> > > >> > >> >
