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: [email protected]
> For additional commands, e-mail: [email protected]
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
