vaadin / multiplatform-runtime

4 stars 1 forks source link

AbstractLegacyWrapper WeakHashMap usage can lead to application hang #116

Closed alasdairg closed 1 year ago

alasdairg commented 1 year ago

MPR Core 2.2.10 JDK 8

The AbstractLegacyWrapper class declares a static WeakHashMap which maps between vaadin.ui.Component and AbstractLegacyWrapper.

Unfortunately, WeakHashMap is not thread-safe, and non-synchronized access can lead to one thread acquiring the lock inside WeakHashMap.expungeStaleEntries() but being trapped forever in the while loop in that method. This leads to all subsequent threads attempting to access the WeakHashMap instance being blocked ... eventually leading to thread starvation. Each construction of an AbstractLegacyWrapper in the Vaadin application accesses the map without synchronization, and we have encountered this thread starvation scenario in a live environment.

java.lang.Thread.State: BLOCKED (on object monitor)
at java.util.WeakHashMap.expungeStaleEntries(WeakHashMap.java:319)
- waiting to lock <0x000000075be23b78> (a java.lang.ref.ReferenceQueue)
at java.util.WeakHashMap.getTable(WeakHashMap.java:350)
at java.util.WeakHashMap.put(WeakHashMap.java:450)
at com.vaadin.mpr.core.AbstractLegacyWrapper.<init>(AbstractLegacyWrapper.java:74)
at com.vaadin.mpr.LegacyWrapper.<init>(LegacyWrapper.java:44)
at ...
at ...
at ...
at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.sendBeforeEnterEvent(AbstractNavigationStateRenderer.java:651)
at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.sendBeforeEnterEvent(AbstractNavigationStateRenderer.java:627)
at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.sendBeforeEnterEventAndPopulateChain(AbstractNavigationStateRenderer.java:537)
at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.createChainIfEmptyAndExecuteBeforeEnterNavigation(AbstractNavigationStateRenderer.java:508)
at com.vaadin.flow.router.internal.AbstractNavigationStateRenderer.handle(AbstractNavigationStateRenderer.java:225)
at com.vaadin.flow.router.Router.handleNavigation(Router.java:261)
at com.vaadin.flow.router.Router.navigate(Router.java:232)
at com.vaadin.flow.router.Router.navigate(Router.java:198)
at com.vaadin.flow.router.Router.initializeUI(Router.java:95)
at com.vaadin.flow.server.BootstrapHandler.initializeUIWithRouter(BootstrapHandler.java:1634)
at com.vaadin.flow.server.BootstrapHandler.createAndInitUI(BootstrapHandler.java:1627)
at com.vaadin.flow.server.BootstrapHandler.synchronizedHandleRequest(BootstrapHandler.java:508)
at com.vaadin.flow.server.SynchronizedRequestHandler.handleRequest(SynchronizedRequestHandler.java:40)
at com.vaadin.flow.server.VaadinService.handleRequest(VaadinService.java:1579)
at com.vaadin.flow.server.VaadinServlet.service(VaadinServlet.java:252)

A quick internet search shows that various other (unrelated) projects have hit this same issue in the past. For example:

https://github.com/spring-projects/spring-data-solr/issues/305 https://bugs.openjdk.org/browse/JDK-8028277 https://issues.apache.org/jira/browse/CALCITE-1594

And I found a useful summary of the problem on this blog:

http://dowbecki.com/WeakHashMap-is-not-thread-safe/

Access to this WeakHashMap should be synchronized. One solution could be to wrap it as a synchronized map:

protected static Map<com.vaadin.ui.Component, WeakReference<AbstractLegacyWrapper>> legacyMap = Collections.synchronizedMap(new WeakHashMap<>());