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: [email protected]
>> For additional commands, e-mail: [email protected]
>>
>>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: [email protected]
> For additional commands, e-mail: [email protected]
>
>
Gruß
Bernd
—
https://bernd.eckenfels.net
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
