From e6af0f0a5a1333d06e18d0149d44231a5f8e654d Mon Sep 17 00:00:00 2001 From: Jonatan Kronqvist Date: Wed, 28 Aug 2013 13:53:45 +0300 Subject: [PATCH] Avoid leaking memory from inherited ThreadLocales. Fixes #12401 The issue is fixed by changing the normal HashMap inside the inheritable thread local to a map implementation holding only weak references to the values (WeakValueMap). Also included is a test UI that starts threads, which run until the JVM is quit. This along with VisualVM was used to reproduce the issue and verify the fix. Change-Id: I116cc4e56e8a19c3b770abab6b18b9e262f4dafa --- .../src/com/vaadin/util/CurrentInstance.java | 2 +- server/src/com/vaadin/util/WeakValueMap.java | 228 ++++++++++++++++++ .../performance/ThreadMemoryLeaksTest.java | 57 +++++ 3 files changed, 286 insertions(+), 1 deletion(-) create mode 100644 server/src/com/vaadin/util/WeakValueMap.java create mode 100644 uitest/src/com/vaadin/tests/performance/ThreadMemoryLeaksTest.java diff --git a/server/src/com/vaadin/util/CurrentInstance.java b/server/src/com/vaadin/util/CurrentInstance.java index b97bab3d8a..a1c543117d 100644 --- a/server/src/com/vaadin/util/CurrentInstance.java +++ b/server/src/com/vaadin/util/CurrentInstance.java @@ -60,7 +60,7 @@ public class CurrentInstance implements Serializable { return null; } - Map, CurrentInstance> value = new HashMap, CurrentInstance>(); + Map, CurrentInstance> value = new WeakValueMap, CurrentInstance>(); // Copy all inheritable values to child map for (Entry, CurrentInstance> e : parentValue.entrySet()) { diff --git a/server/src/com/vaadin/util/WeakValueMap.java b/server/src/com/vaadin/util/WeakValueMap.java new file mode 100644 index 0000000000..b5ac12ce91 --- /dev/null +++ b/server/src/com/vaadin/util/WeakValueMap.java @@ -0,0 +1,228 @@ +/* + * Copyright 2000-2013 Vaadin Ltd. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not + * use this file except in compliance with the License. You may obtain a copy of + * the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package com.vaadin.util; + +import java.lang.ref.Reference; +import java.lang.ref.ReferenceQueue; +import java.lang.ref.WeakReference; +import java.util.AbstractMap; +import java.util.Collection; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; + +/** + * A Map holding weak references to its values. It is internally backed by a + * normal HashMap and all values are stored as WeakReferences. Garbage collected + * entries are removed when touched. + * + * @author Vaadin Ltd + * @since 7.1.4 + */ +public class WeakValueMap implements Map { + + /** + * This class holds a weak reference to the value and a strong reference to + * the key for efficient removal of stale values. + */ + private static class WeakValueReference extends WeakReference { + private final K key; + + WeakValueReference(K key, V value, ReferenceQueue refQueue) { + super(value, refQueue); + this.key = key; + } + + K getKey() { + return key; + } + } + + private final HashMap> backingMap; + private final ReferenceQueue refQueue; + + /** + * Constructs a new WeakValueMap, where all values are stored as weak + * references. + */ + public WeakValueMap() { + backingMap = new HashMap>(); + refQueue = new ReferenceQueue(); + } + + /** + * {@inheritDoc} + */ + @Override + public V put(K key, V value) { + if (key == null) { + throw new NullPointerException("key cannot be null"); + } + if (value == null) { + throw new NullPointerException("value cannot be null"); + } + removeStaleEntries(); + backingMap.put(key, new WeakValueReference(key, value, refQueue)); + return value; + } + + /** + * {@inheritDoc} + */ + @Override + public V remove(Object o) { + removeStaleEntries(); + WeakReference value = backingMap.remove(o); + return value == null ? null : value.get(); + } + + /** + * {@inheritDoc} + */ + @Override + public void putAll(Map map) { + if (map != null) { + for (Map.Entry entry : map.entrySet()) { + put(entry.getKey(), entry.getValue()); + } + } + } + + /** + * {@inheritDoc} + */ + @Override + public void clear() { + backingMap.clear(); + } + + /** + * {@inheritDoc} + */ + @Override + public Set keySet() { + removeStaleEntries(); + return backingMap.keySet(); + } + + /** + * {@inheritDoc} + */ + @Override + public V get(Object o) { + removeStaleEntries(); + WeakReference weakValue = backingMap.get(o); + if (weakValue != null) { + return weakValue.get(); + } + return null; + } + + /** + * {@inheritDoc} + */ + @Override + public int size() { + removeStaleEntries(); + return backingMap.size(); + } + + /** + * {@inheritDoc} + */ + @Override + public boolean isEmpty() { + removeStaleEntries(); + return backingMap.isEmpty(); + } + + /** + * {@inheritDoc} + */ + @Override + public boolean containsKey(Object o) { + removeStaleEntries(); + return backingMap.containsKey(o); + } + + /** + * {@inheritDoc} + */ + @Override + public boolean containsValue(Object o) { + removeStaleEntries(); + for (V value : values()) { + if (o.equals(value)) { + return true; + } + } + return false; + } + + /** + * {@inheritDoc} + */ + @Override + public Collection values() { + removeStaleEntries(); + Collection values = new HashSet(); + for (WeakReference weakValue : backingMap.values()) { + V value = weakValue.get(); + if (value != null) { + // null values have been GC'd, which may happen long before + // anything is enqueued in the ReferenceQueue. + values.add(value); + } + } + return values; + } + + /** + * {@inheritDoc} + */ + @Override + public Set> entrySet() { + removeStaleEntries(); + Set> entrySet = new HashSet>(); + for (Entry> entry : backingMap.entrySet()) { + V value = entry.getValue().get(); + if (value != null) { + // null values have been GC'd, which may happen long before + // anything is enqueued in the ReferenceQueue. + entrySet.add(new AbstractMap.SimpleEntry(entry.getKey(), + value)); + } + } + return entrySet; + } + + /** + * Cleans up stale entries by polling the ReferenceQueue. + *

+ * Depending on the GC implementation and strategy, the ReferenceQueue is + * not necessarily notified immediately when a reference is garbage + * collected, but it will eventually be. + */ + private void removeStaleEntries() { + Reference ref; + while ((ref = refQueue.poll()) != null) { + Object key = ((WeakValueReference) ref).getKey(); + backingMap.remove(key); + } + } +} diff --git a/uitest/src/com/vaadin/tests/performance/ThreadMemoryLeaksTest.java b/uitest/src/com/vaadin/tests/performance/ThreadMemoryLeaksTest.java new file mode 100644 index 0000000000..5ffc7141af --- /dev/null +++ b/uitest/src/com/vaadin/tests/performance/ThreadMemoryLeaksTest.java @@ -0,0 +1,57 @@ +package com.vaadin.tests.performance; + +import java.util.Date; +import java.util.Timer; +import java.util.TimerTask; + +import com.vaadin.server.VaadinRequest; +import com.vaadin.tests.components.AbstractTestUI; +import com.vaadin.ui.Button; +import com.vaadin.ui.Label; + +public class ThreadMemoryLeaksTest extends AbstractTestUI { + + public static class Worker { + long value = 0; + private TimerTask task = new TimerTask() { + @Override + public void run() { + value++; + } + }; + private final Timer timer = new Timer(true); + + public Worker() { + timer.scheduleAtFixedRate(task, new Date(), 1000); + } + } + + int workers = 0; + Label label; + + @Override + protected void setup(VaadinRequest request) { + label = new Label(String.format("%d workers", workers)); + addComponent(label); + addComponent(new Button("Add worker", new Button.ClickListener() { + @Override + public void buttonClick(Button.ClickEvent event) { + new Worker(); + workers++; + label.setValue(String.format("%d workers", workers)); + } + })); + } + + @Override + protected String getTestDescription() { + return "Inherited ThreadLocals should not leak memory. Clicking the " + + "button starts a new thread, after which memory consumption " + + "can be checked with visualvm"; + } + + @Override + protected Integer getTicketNumber() { + return 12401; + } +} -- 2.39.5