search

vaadin / vaadin-combo-box-flow

Vaadin Flow Java API for vaadin/vaadin-combo-box Web Component
https://vaadin.com/components/vaadin-combo-box
Other
5 stars 9 forks source link

Explicitly remove old dataprovider listener when new one is set #382

Closed TatuLund closed 4 years ago

TatuLund commented 4 years ago

Explicitly remove old dataprovider listener when new one is set, not doing so can cause memory leakage

Fixes: https://github.com/vaadin/vaadin-combo-box-flow/issues/383

CLAassistant commented 4 years ago

CLA assistant check
All committers have signed the CLA.

heruan commented 4 years ago

Is this enough to solve the leak? If the ComboBox is detached, the listener still holds a reference to it in the DataProvider.

TatuLund commented 4 years ago

Is this enough to solve the leak? If the ComboBox is detached, the listener still holds a reference to it in the DataProvider.

That is indeed a possibility that complicates matters, I updated the PR to take this into account and scenarios due that.

eriklumme commented 4 years ago

Two concerns:

  1. Correct me if I'm wrong, but I believe with the current implementation, the listener would be removed on detach, and removed again when re-attached (since it's never set to null), which I believe would throw an error from the registration.

  2. With this solution, it won't refresh when detached. This will lead to problems when a combo box is temporarily detached (say hidden), but it is still desired that it shows the refreshed data when re-attached. Maybe this is acceptable, another option is to always refresh on attach, but this might hurt performance.

TatuLund commented 4 years ago

@eriklumme Thanks for pointing out. I took those into account as well.

heruan commented 4 years ago

2. With this solution, it won't refresh when detached. This will lead to problems when a combo box is temporarily detached (say hidden), but it is still desired that it shows the refreshed data when re-attached.

This would be a problem for us, indeed: detaching seems not the right event to free the listener.

TatuLund commented 4 years ago

This would be a problem for us, indeed: detaching seems not the right event to free the listener.

@heruan yes, I anticipated that and hence I corrected the both points mentioned by Erik in the patch. As you see, the data is refreshed on attach when needed.

heruan commented 4 years ago

Thanks Tatu! I'm testing this, the memory issue is reduced but still I see a large number of lambdas in the heap—I'm not sure how this will behave in production. Seems like the detach event might be missed in certain cases: maybe when the browser tab is closed or the address changed?

Silly question probably: are we sure the listener is needed at all? I mean: other components with data-providers don't set listeners or at least don't clutter the heap, so maybe there is another way around that?

TatuLund commented 4 years ago

Silly question probably: are we sure the listener is needed at all?

There is a comment in Vaadin 8 version of the ComboBox that sort of explains it, I do not know why such comment has not been included in Vaadin 14 version.

https://github.com/vaadin/framework/blob/master/server/src/main/java/com/vaadin/ui/ComboBox.java#L995

heruan commented 4 years ago

Thanks Tatu! The comment says that's a workaround for in-memory and unpaged data-providers, but it seems here it's applied regardless of the data-provider type.