Skip to content

Commit 9daf03f

Browse files
vursenvursen
authored
fix: Release no longer visible items from memory (#22185)
This change ensures that items are removed from the HierarchicalDataCommunicator's cache and properly disposed of in both KeyMapper and DataGenerator once they move out of the viewport. Without this, such features as ComponentRenderer can cause high memory usage on the client side because the Flow client continues to retain references to DOM elements that are no longer visible. Related-to: #21876
1 parent d827ba7 commit 9daf03f

File tree

6 files changed

+290
-77
lines changed

6 files changed

+290
-77
lines changed

‎flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/Cache.java

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@
4444
class Cache<T> implements Serializable {
4545
private final RootCache<T> rootCache;
4646
private final Cache<T> parentCache;
47-
private final int parentIndex;
47+
private final T parentItem;
4848
private int size;
4949

5050
private final Map<Object, T> itemIdToItem = new HashMap<>();
@@ -57,26 +57,26 @@ class Cache<T> implements Serializable {
5757
*
5858
* @param parentCache
5959
* the parent cache, or {@code null} if this is the root cache
60-
* @param parentIndex
61-
* the index of this cache in the parent cache
60+
* @param parentItem
61+
* the parent item, or {@code null} if this is the root cache
6262
* @param size
6363
* the size of this cache
6464
*/
65-
protected Cache(Cache<T> parentCache, int parentIndex, int size) {
65+
protected Cache(Cache<T> parentCache, T parentItem, int size) {
6666
this.rootCache = parentCache != null ? parentCache.rootCache
6767
: (RootCache<T>) this;
6868
this.parentCache = parentCache;
69-
this.parentIndex = parentIndex;
69+
this.parentItem = parentItem;
7070
this.size = size;
7171
}
7272

7373
/**
74-
* Gets the item in the parent cache that this cache is associated with.
74+
* Gets the parent item this cache is associated with from the parent cache.
7575
*
76-
* @return the parent item or {@code null} if there is no parent cache
76+
* @return the parent item or {@code null} if this is the root cache
7777
*/
7878
public T getParentItem() {
79-
return parentCache != null ? parentCache.getItem(parentIndex) : null;
79+
return parentItem;
8080
}
8181

8282
/**
@@ -128,6 +128,18 @@ public T getItem(int index) {
128128
return itemIdToItem.get(itemId);
129129
}
130130

131+
/**
132+
* Removes the item at the specified index from this cache.
133+
*
134+
* @param index
135+
* the index of the item to remove
136+
*/
137+
public void removeItem(int index) {
138+
var itemId = indexToItemId.remove(index);
139+
var item = itemIdToItem.remove(itemId);
140+
rootCache.removeItemContext(item);
141+
}
142+
131143
/**
132144
* Replaces a cached item with a new instance. Items are matched by their
133145
* IDs in the data provider.
@@ -220,10 +232,10 @@ public Set<Entry<Integer, Cache<T>>> getSubCaches() {
220232
* a supplier that provides the size of the new sub-cache
221233
* @return the sub-cache instance
222234
*/
223-
public Cache<T> ensureSubCache(int index,
235+
public Cache<T> ensureSubCache(int index, T item,
224236
SerializableSupplier<Integer> sizeSupplier) {
225237
return indexToCache.computeIfAbsent(index,
226-
(_key) -> new Cache<>(this, index, sizeSupplier.get()));
238+
(_key) -> new Cache<>(this, item, sizeSupplier.get()));
227239
}
228240

229241
/**

‎flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicator.java

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.util.Objects;
2626
import java.util.Optional;
2727
import java.util.Set;
28+
import java.util.stream.Collectors;
2829
import java.util.stream.Stream;
2930

3031
import com.fasterxml.jackson.databind.JsonNode;
@@ -504,7 +505,7 @@ private void resolveIndexPath(Cache<T> cache, int... path) {
504505
var item = cache.getItem(index);
505506
if (getHierarchyFormat().equals(HierarchyFormat.NESTED)
506507
&& isExpanded(item)) {
507-
var subCache = cache.ensureSubCache(index, () -> {
508+
var subCache = cache.ensureSubCache(index, item, () -> {
508509
requestFlush().invalidateViewport();
509510
return getDataProviderChildCount(item);
510511
});
@@ -567,7 +568,7 @@ protected List<T> preloadFlatRangeBackward(int start, int length) {
567568
if (getHierarchyFormat().equals(HierarchyFormat.NESTED)
568569
&& isExpanded(item) && !cache.hasSubCache(index)
569570
&& result.size() > 0) {
570-
var subCache = cache.ensureSubCache(index, () -> {
571+
var subCache = cache.ensureSubCache(index, item, () -> {
571572
requestFlush().invalidateViewport();
572573
return getDataProviderChildCount(item);
573574
});
@@ -618,7 +619,7 @@ protected List<T> preloadFlatRangeForward(int start, int length) {
618619
var item = cache.getItem(index);
619620
if (getHierarchyFormat().equals(HierarchyFormat.NESTED)
620621
&& isExpanded(item)) {
621-
cache.ensureSubCache(index, () -> {
622+
cache.ensureSubCache(index, item, () -> {
622623
requestFlush().invalidateViewport();
623624
return getDataProviderChildCount(item);
624625
});
@@ -663,7 +664,9 @@ private void flush(ExecutionContext context) {
663664
var start = viewportRange.getStart();
664665
var end = viewportRange.getEnd();
665666

666-
var result = preloadFlatRangeForward(start, length);
667+
var viewportItems = preloadFlatRangeForward(start, length);
668+
var viewportItemIds = viewportItems.stream()
669+
.map(getDataProvider()::getId).collect(Collectors.toSet());
667670

668671
var flatSize = rootCache.getFlatSize();
669672

@@ -674,17 +677,27 @@ private void flush(ExecutionContext context) {
674677
if (end < flatSize) {
675678
update.clear(end, flatSize - end);
676679
}
677-
for (int i = 0; i < result.size(); i++) {
678-
var item = result.get(i);
680+
for (int i = 0; i < viewportItems.size(); i++) {
681+
var item = viewportItems.get(i);
679682
var index = start + i;
680683

684+
// Send updates only for items that are new in the viewport,
685+
// whose data has changed, or when the entire viewport needs
686+
// to be updated.
681687
if (flushRequest.isViewportInvalidated()
682688
|| flushRequest.isItemInvalidated(item)
683689
|| flushRequest.isIndexInvalidated(index)) {
684690
update.set(index, List.of(generateItemJson(item)));
685691
}
686692
}
687693
update.commit(nextUpdateId++);
694+
695+
// Clear items that are no longer visible from the cache to free memory.
696+
// This also clears them from keyMapper and disposes of their generated
697+
// data in dataGenerator to avoid memory leaks on both server and
698+
// client side.
699+
rootCache.removeDescendantItemIf((item) -> getKeyMapper().has(item)
700+
&& !viewportItemIds.contains(getDataProvider().getId(item)));
688701
}
689702

690703
private HierarchyFormat getHierarchyFormat() {
@@ -772,8 +785,10 @@ private RootCache<T> ensureRootCache() {
772785
void removeItemContext(T item) {
773786
super.removeItemContext(item);
774787

775-
getKeyMapper().remove(item);
776-
dataGenerator.destroyData(item);
788+
if (getKeyMapper().has(item)) {
789+
dataGenerator.destroyData(item);
790+
getKeyMapper().remove(item);
791+
}
777792
}
778793
};
779794
}

‎flow-data/src/main/java/com/vaadin/flow/data/provider/hierarchy/RootCache.java

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import java.util.Map;
2222
import java.util.Map.Entry;
2323

24+
import com.vaadin.flow.function.SerializablePredicate;
2425
import com.vaadin.flow.function.ValueProvider;
2526

2627
/**
@@ -50,7 +51,7 @@ class RootCache<T> extends Cache<T> {
5051
* the item ID provider
5152
*/
5253
public RootCache(int size, ValueProvider<T, Object> itemIdProvider) {
53-
super(null, -1, size);
54+
super(null, null, size);
5455
this.itemIdProvider = itemIdProvider;
5556
}
5657

@@ -190,6 +191,25 @@ public ItemContext<T> getContextByItem(T item) {
190191
return itemIdToContext.get(itemId);
191192
}
192193

194+
/**
195+
* Removes all descendant items that match the given predicate.
196+
*
197+
* @param predicate
198+
* the predicate to match items against
199+
*/
200+
public void removeDescendantItemIf(SerializablePredicate<T> predicate) {
201+
itemIdToContext.values().stream().filter((itemContext) -> {
202+
var cache = itemContext.cache();
203+
var index = itemContext.index();
204+
var item = cache.getItem(index);
205+
return predicate.test(item);
206+
}).toList().forEach((itemContext) -> {
207+
var cache = itemContext.cache();
208+
var index = itemContext.index();
209+
cache.removeItem(index);
210+
});
211+
}
212+
193213
void addItemContext(T item, Cache<T> cache, int index) {
194214
Object itemId = getItemId(item);
195215
itemIdToContext.put(itemId, new ItemContext<>(cache, index));

‎flow-data/src/test/java/com/vaadin/flow/data/provider/hierarchy/HierarchicalDataCommunicatorBasicTest.java

Lines changed: 8 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -6,22 +6,19 @@
66
import java.util.List;
77
import java.util.stream.Stream;
88

9-
import com.fasterxml.jackson.databind.node.ObjectNode;
109
import org.junit.Assert;
1110
import org.junit.Before;
1211
import org.junit.Test;
1312
import org.mockito.Mockito;
1413

1514
import com.vaadin.flow.data.provider.CompositeDataGenerator;
16-
import com.vaadin.flow.data.provider.DataGenerator;
1715
import com.vaadin.flow.data.provider.DataProvider;
1816
import com.vaadin.flow.data.provider.ListDataProvider;
1917
import com.vaadin.flow.data.provider.QuerySortOrder;
2018
import com.vaadin.flow.data.provider.QuerySortOrderBuilder;
2119
import com.vaadin.flow.data.provider.SortDirection;
2220
import com.vaadin.flow.dom.Element;
2321
import com.vaadin.flow.function.SerializableComparator;
24-
import com.vaadin.flow.function.SerializableConsumer;
2522
import com.vaadin.flow.function.SerializablePredicate;
2623

2724
public class HierarchicalDataCommunicatorBasicTest
@@ -175,13 +172,14 @@ public boolean hasChildren(Item item) {
175172
}
176173

177174
@Test
178-
public void getDepth_returnsDepthForCachedItemsAfterTheyAreLoaded() {
175+
public void getDepth_returnsDepthForViewportItems() {
179176
populateTreeData(treeData, 100, 1, 1);
180177
dataCommunicator.setDataProvider(treeDataProvider, null);
181178
dataCommunicator.expand(
182179
Arrays.asList(new Item("Item 0"), new Item("Item 0-0")));
183180
dataCommunicator.setViewportRange(0, 4);
184181

182+
// Not loaded yet
185183
Assert.assertEquals(-1, dataCommunicator.getDepth(new Item("Item 0")));
186184
Assert.assertEquals(-1,
187185
dataCommunicator.getDepth(new Item("Item 0-0")));
@@ -191,6 +189,7 @@ public void getDepth_returnsDepthForCachedItemsAfterTheyAreLoaded() {
191189

192190
fakeClientCommunication();
193191

192+
// Loaded
194193
Assert.assertEquals(0, dataCommunicator.getDepth(new Item("Item 0")));
195194
Assert.assertEquals(1, dataCommunicator.getDepth(new Item("Item 0-0")));
196195
Assert.assertEquals(2,
@@ -200,7 +199,11 @@ public void getDepth_returnsDepthForCachedItemsAfterTheyAreLoaded() {
200199
dataCommunicator.setViewportRange(4, 4);
201200
fakeClientCommunication();
202201

203-
Assert.assertEquals(1, dataCommunicator.getDepth(new Item("Item 0-0")));
202+
// Out of new viewport
203+
Assert.assertEquals(-1,
204+
dataCommunicator.getDepth(new Item("Item 0-0")));
205+
206+
// Within new viewport
204207
Assert.assertEquals(0, dataCommunicator.getDepth(new Item("Item 5")));
205208
}
206209

@@ -255,55 +258,6 @@ public void collapseCollectionOfItems_returnsEffectivelyCollapsedItems() {
255258
Arrays.asList(new Item("Item 0"), new Item("Item 1"))));
256259
}
257260

258-
@Test
259-
public void collapseItems_collapsedChildrenRemovedFromKeyMapper() {
260-
populateTreeData(treeData, 100, 2, 2);
261-
dataCommunicator.setDataProvider(treeDataProvider, null);
262-
dataCommunicator.expand(new Item("Item 0"));
263-
dataCommunicator.setViewportRange(0, 6);
264-
fakeClientCommunication();
265-
Assert.assertTrue(
266-
dataCommunicator.getKeyMapper().has(new Item("Item 0-0")));
267-
Assert.assertTrue(
268-
dataCommunicator.getKeyMapper().has(new Item("Item 0-1")));
269-
270-
Mockito.clearInvocations(arrayUpdater, arrayUpdate);
271-
272-
dataCommunicator.collapse(new Item("Item 0"));
273-
fakeClientCommunication();
274-
Assert.assertFalse(
275-
dataCommunicator.getKeyMapper().has(new Item("Item 0-0")));
276-
Assert.assertFalse(
277-
dataCommunicator.getKeyMapper().has(new Item("Item 0-1")));
278-
}
279-
280-
@Test
281-
public void collapseItems_dataGeneratorDestroyDataCalledForCollapsedChildren() {
282-
populateTreeData(treeData, 4, 2, 2);
283-
dataCommunicator.setDataProvider(treeDataProvider, null);
284-
dataCommunicator.expand(
285-
Arrays.asList(new Item("Item 0"), new Item("Item 0-0")));
286-
dataCommunicator.setViewportRange(0, 4);
287-
fakeClientCommunication();
288-
289-
var dataGenerator = Mockito.spy(new DataGenerator<Item>() {
290-
@Override
291-
public void generateData(Item item, ObjectNode json) {
292-
// NO-OP
293-
}
294-
});
295-
compositeDataGenerator.addDataGenerator(dataGenerator);
296-
297-
dataCommunicator.collapse(new Item("Item 0"));
298-
299-
Mockito.verify(dataGenerator, Mockito.never())
300-
.destroyData(new Item("Item 0"));
301-
Mockito.verify(dataGenerator).destroyData(new Item("Item 0-0"));
302-
Mockito.verify(dataGenerator).destroyData(new Item("Item 0-1"));
303-
Mockito.verify(dataGenerator).destroyData(new Item("Item 0-0-0"));
304-
Mockito.verify(dataGenerator).destroyData(new Item("Item 0-0-1"));
305-
}
306-
307261
@Test
308262
public void setFilterViaDataProvider_filterApplied() {
309263
populateTreeData(treeData, 3, 2, 1);

0 commit comments

Comments
 (0)