TY Bernd!
Gary
On Mon, May 27, 2024 at 12:33 PM Bernd Eckenfels <e...@zusammenkunft.net> wrote:
>
> Hello,
>
> I pushed a PR which fixes the issue and
> Contains a test (which fails if the fix is not
> Present. Besides the uneeded double
> Indirection this code also leads to prematurely
> Dropped listeners and therefore loses change events.
>
> Unfortunately a CI Acrion on Girhub failed with a unrelated sporadic problem
> and I don’t have permission to restart it or integrate the PR. Can somebody
> go the needful or let me know if I
> Need a dummy push.
>
> Gruß
> Bernd
>
> Gary D. Gregory wrote on 23. May 2024 20:00 (GMT +02:00):
>
> > Hi Bernd,
> >
> > Thank you for researching this issue and presenting your findings.
> >
> > In 2.9.0, we had (as you found):
> >
> > public static void installListener(final FileObject file, final
> > FileListener listener) {
> > final WeakRefFileListener weakListener = new
> > WeakRefFileListener(file, listener);
> >
> > file.getFileSystem().addListener(file, new
> > WeakRefFileListener(file, weakListener));
> > }
> >
> > Which already contains the "WeakRefFileListener in a WeakRefFileListener".
> >
> > Unless I am missing something, the change in git master now just in
> > inlines the local variable.
> >
> > Are we saying that:
> > (1) We should not have a "WeakRefFileListener in a WeakRefFileListener"
> > (2) This problem already existed in 2.9.0.
> > (3) This problem existed when the class was introduced in 2.2 (despite the
> > 2.0 Javadoc since tag).
> >
> > Let me know how you see it.
> >
> > I can't say if your proposed change has any unintended side-effects; it
> > should be accompanied with some kind of test, at the very least to avoid a
> > regression.
> >
> > TY!
> > Gary
> >
> >
> > On 2024/05/23 17:08:00 Bernd Eckenfels wrote:
> >> Hello,
> >>
> >> I am dealing with a heapdump of VFS where I see a lot of
> >> WeakRefFileListener
> >> and all of them have a empty WeakRef to no listener. While I think I
> >> found the
> >> reason for that and fixed it on a dependent project, it still does not
> >> clean
> >> up correctly. I think the reason is that it does not store the
> >> WeakRefListener
> >> directly, but it stores a WeakReflistener of it. This will then
> >> immediatelly
> >> result in a unreferencedreferent - so it never works (it surely does fix
> >> the leak :)
> >>
> >> Gary, while cleaning up lint errors in fba04f3e5 you made a change, but I
> >> asume it was
> >> only a mechanical replacement - or did you actually checkedif it is
> >> correct?
> >>
> >>
> >> https://github.com/apache/commons-vfs/blob/dc9ad7677a020b2d4c571f7dcc858cdbae2bb538/commons-vfs2/src/main/java/org/apache/commons/vfs2/util/WeakRefFileListener.java#L41
> >>
> >> Cleaned up code:
> >> public static void installListener(final FileObject file, final
> >> FileListener listener) {
> >> file.getFileSystem().addListener(file, new WeakRefFileListener(file,
> >> new WeakRefFileListener(file, listener)));
> >> }
> >>
> >> initial version:
> >> final WeakRefFileListener weakListener = new WeakRefFileListener(file,
> >> listener);
> >> file.getFileSystem().addListener(file, new WeakRefFileListener(file,
> >> weakListener));
> >>
> >>
> >> but I think it should be only a single wrapper:
> >>
> >> public static void installListener(final FileObject file, final
> >> FileListener listener) {
> >> file.getFileSystem().addListener(file, new WeakRefFileListener(file,
> >> listener));
> >> }
> >>
> >> There is a mention of VFS-143, but itintroduced the whole code with the
> >> double indirection
> >> and it does not explain why it is needed.
> >>
> >> What do you think, should we change it?
> >>
> >> I also wonder why no tests sees it (in factI try to add a test to
> >> reproduce what I think
> >> shows its not working).
> >>
> >> Gruss
> >> Bernd
> >>
> >> ---------------------------------------------------------------------
> >> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> >> For additional commands, e-mail: dev-h...@commons.apache.org
> >>
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> > For additional commands, e-mail: dev-h...@commons.apache.org
> >
> >
>
>
> Gruß
> Bernd
> —
> https://bernd.eckenfels.net
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
> For additional commands, e-mail: dev-h...@commons.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
