Stephan, agree that it is not a real memory leak. I haven't found it affecting the system. so it is sth odd for now.
but if it is not really necessary, why do we want to defer memory release with unpredictable behavior? can StreamTask stop() method take care of the cleanup work and don't need to rely on finalizer() or PhantomReference? On Tue, Sep 19, 2017 at 2:56 AM, Stephan Ewen <se...@apache.org> wrote: > Hi! > > From my understanding, overriding finalize() still has some use cases and > is valid if done correctly, (although PhantomReference has more control > over the cleanup process). finalize() is still used in JDK classes as well. > > Whenever one overrides finalize(), the object cannot be immediately > garbage collected because the finalize() method may make it reachable > again. It results in the following life cycle: > > 1) object becomes unreachable, is detected eligible for GC > 2) In the GC cycle, object is NOT collected, but finalize() is called > 3) If object is still not reachable, it will be collected in the > subsequent GC cycle > > In essence, objects that override finalize() stay for one more GC cycle. > That may be what you are seeing. It should not be a real memory leak, but > deferred memory release. > > Is this a problem that is affecting the system, or only something that > seems odd for now? > > If you are very concerned about this, would you be up to contribute a > change that uses a PhantomReference and Reference Queue for cleanup instead? > > Stephan > > > On Tue, Sep 19, 2017 at 12:56 AM, Till Rohrmann <trohrm...@apache.org> > wrote: > >> Hi Steven, >> >> the finalize method in StreamTask acts as a safety net in case the >> services of the StreamTask haven't been properly shut down. In the code, >> however, it looks as if the TimerService, for example, is always being >> stopped in the finally block of the invoke method. Thus, it might not be >> necessary to have the finalize method as a safety net. >> >> How did you kill the TaskManagers? I assume you didn't kill the JVM >> process because otherwise you wouldn't see the finalizer objects piling up. >> >> I think that you can create a JIRA issue for removing the finalizer >> method. >> >> Cheers, >> Till >> >> >> >> On Thu, Sep 14, 2017 at 12:26 PM, Fabian Hueske <fhue...@gmail.com> >> wrote: >> >>> Hi Steven, >>> >>> thanks for reporting this issue. >>> Looping in Till who's more familiar with the task lifecycles. >>> >>> Thanks, Fabian >>> >>> 2017-09-12 7:08 GMT+02:00 Steven Wu <stevenz...@gmail.com>: >>> >>>> Hi , >>>> >>>> I was using Chaos Monkey to test Flink's behavior against frequent >>>> killing of task manager nodes. I found that stopped/disposed StreamTask got >>>> retained by java finalizer. It is kind like a memory leak. Since each >>>> StreamTask retains 2.6 MB memory. With 20 kills (and job restarts) for >>>> 8-CPU container, there are 2.6 * 20 * 8 MB retained in heap. >>>> >>>> [image: Inline image 1] >>>> >>>> finalize() is generally not recommended for cleanup, because "*Finalizers >>>> are unpredictable, often dangerous, and generally unnecessary*", >>>> quoted from Joshua Bloch's book. >>>> http://www.informit.com/articles/article.aspx?p=1216151&seqNum=7 >>>> >>>> This code from StreamTask.java seems to be the cause. Is it necessary? >>>> can it be removed? We are using flink-1.2 release branch. But I see the >>>> same code in flink-1.3 and master branch >>>> >>>> /** >>>> * The finalize method shuts down the timer. This is a fail-safe >>>> shutdown, in case the original >>>> * shutdown method was never called. >>>> * >>>> * <p> >>>> * This should not be relied upon! It will cause shutdown to happen much >>>> later than if manual >>>> * shutdown is attempted, and cause threads to linger for longer than >>>> needed. >>>> */ >>>> @Override >>>> protected void finalize() throws Throwable { >>>> super.finalize(); >>>> if (timerService != null) { >>>> if (!timerService.isTerminated()) { >>>> LOG.info("Timer service is shutting down."); >>>> timerService.shutdownService(); >>>> } >>>> } >>>> >>>> cancelables.close(); >>>> } >>>> >>>> Thanks, >>>> Steven >>>> >>> >>> >> >
