Skip to content

Commit

Permalink
fix: remove detach listener when javascript execution completes (#16836…
Browse files Browse the repository at this point in the history 
…) (#16848)

* fix: remove detach listener when javascript execution completes

A detach listener is added for pending javascript invocation owner nodes
to clean up the queue, if the owner gets detached.
This change also removes the detach listener when the javascript execution
completes.

* make sure only one detach listener per node is registered

* change map type for detach listeners

Co-authored-by: Marco Collovati <marco@vaadin.com>
  • Loading branch information
@vaadin-bot @mcollovati
vaadin-bot and mcollovati committed May 23, 2023
1 parent f469f56 commit 38cb83b
Show file tree
Hide file tree
Showing 2 changed files with 206 additions and 6 deletions.
63 changes: 57 additions & 6 deletions flow-server/src/main/java/com/vaadin/flow/component/internal/UIInternals.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,10 @@
import com.vaadin.flow.component.page.Page;
import com.vaadin.flow.dom.ElementUtil;
import com.vaadin.flow.dom.impl.BasicElementStateProvider;
import com.vaadin.flow.function.SerializableConsumer;
import com.vaadin.flow.internal.ConstantPool;
import com.vaadin.flow.internal.JsonCodec;
import com.vaadin.flow.internal.StateNode;
import com.vaadin.flow.internal.StateTree;
import com.vaadin.flow.internal.UrlUtil;
import com.vaadin.flow.internal.nodefeature.LoadingIndicatorConfigurationMap;
Expand All @@ -70,6 +72,7 @@
import com.vaadin.flow.router.internal.AfterNavigationHandler;
import com.vaadin.flow.router.internal.BeforeEnterHandler;
import com.vaadin.flow.router.internal.BeforeLeaveHandler;
import com.vaadin.flow.server.Command;
import com.vaadin.flow.server.VaadinService;
import com.vaadin.flow.server.VaadinSession;
import com.vaadin.flow.server.communication.PushConnection;
Expand Down Expand Up @@ -170,6 +173,8 @@ public List<Object> getParameters() {

private List<PendingJavaScriptInvocation> pendingJsInvocations = new ArrayList<>();

private final HashMap<StateNode, PendingJavaScriptInvocationDetachListener> pendingJsInvocationDetachListeners = new HashMap<>();

/**
* The related UI.
*/
Expand Down Expand Up @@ -569,11 +574,6 @@ public void addJavaScriptInvocation(
PendingJavaScriptInvocation invocation) {
session.checkHasLock();
pendingJsInvocations.add(invocation);

invocation.getOwner()
.addDetachListener(() -> pendingJsInvocations
.removeIf(pendingInvocation -> pendingInvocation
.equals(invocation)));
}

/**
Expand All @@ -598,10 +598,61 @@ public List<PendingJavaScriptInvocation> dumpPendingJavaScriptInvocations() {
pendingJsInvocations = getPendingJavaScriptInvocations()
.filter(invocation -> !invocation.getOwner().isVisible())
.collect(Collectors.toCollection(ArrayList::new));

pendingJsInvocations
.forEach(this::registerDetachListenerForPendingInvocation);
return readyToSend;
}

@SuppressWarnings({ "rawtypes", "unchecked" })
private void registerDetachListenerForPendingInvocation(
PendingJavaScriptInvocation invocation) {

PendingJavaScriptInvocationDetachListener listener = pendingJsInvocationDetachListeners
.computeIfAbsent(invocation.getOwner(), node -> {
PendingJavaScriptInvocationDetachListener detachListener = new PendingJavaScriptInvocationDetachListener();
detachListener.registration = Registration.combine(
() -> pendingJsInvocationDetachListeners
.remove(node),
node.addDetachListener(detachListener));
return detachListener;
});
listener.invocationList.add(invocation);

SerializableConsumer callback = unused -> listener
.onInvocationCompleted(invocation);
invocation.then(callback, callback);
}

private class PendingJavaScriptInvocationDetachListener implements Command {
private final Set<PendingJavaScriptInvocation> invocationList = new HashSet<>();

private Registration registration;

@Override
public void execute() {
if (!invocationList.isEmpty()) {
List<PendingJavaScriptInvocation> copy = new ArrayList<>(
invocationList);
invocationList.clear();
copy.forEach(this::removePendingInvocation);
}
}

private void removePendingInvocation(
PendingJavaScriptInvocation invocation) {
UIInternals.this.pendingJsInvocations.removeIf(
pendingInvocation -> pendingInvocation.equals(invocation));
if (invocationList.isEmpty() && registration != null) {
registration.remove();
}
}

void onInvocationCompleted(PendingJavaScriptInvocation invocation) {
invocationList.remove(invocation);
removePendingInvocation(invocation);
}
}

/**
* Gets the pending javascript invocations added with
* {@link #addJavaScriptInvocation(PendingJavaScriptInvocation)} after last
Expand Down
149 changes: 149 additions & 0 deletions flow-server/src/test/java/com/vaadin/flow/component/internal/UIInternalsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.mockito.ArgumentMatchers;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
Expand All @@ -24,6 +25,10 @@
import com.vaadin.flow.di.DefaultInstantiator;
import com.vaadin.flow.dom.Element;
import com.vaadin.flow.function.DeploymentConfiguration;
import com.vaadin.flow.internal.JsonCodec;
import com.vaadin.flow.internal.StateNode;
import com.vaadin.flow.internal.nodefeature.ElementChildrenList;
import com.vaadin.flow.internal.nodefeature.ElementData;
import com.vaadin.flow.router.Location;
import com.vaadin.flow.router.ParentLayout;
import com.vaadin.flow.router.Route;
Expand Down Expand Up @@ -274,6 +279,150 @@ public void showRouteTarget_navigateToAnotherLayoutHierarchy_detachedLayoutHiera
subLayout.getElement().getChildren().count());
}

@Test
public void dumpPendingJavaScriptInvocations_detachListenerRegisteredOnce() {
StateNode node = Mockito.spy(new StateNode(ElementData.class));
node.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node);

internals.addJavaScriptInvocation(new PendingJavaScriptInvocation(node,
new UIInternals.JavaScriptInvocation("")));
internals.dumpPendingJavaScriptInvocations();
internals.dumpPendingJavaScriptInvocations();
internals.dumpPendingJavaScriptInvocations();

Mockito.verify(node, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());
}

@Test
public void dumpPendingJavaScriptInvocations_multipleInvocationPerNode_onlyOneDetachListenerRegistered() {
StateNode node = Mockito.spy(new StateNode(ElementData.class));
node.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node);

internals.addJavaScriptInvocation(new PendingJavaScriptInvocation(node,
new UIInternals.JavaScriptInvocation("1")));
internals.addJavaScriptInvocation(new PendingJavaScriptInvocation(node,
new UIInternals.JavaScriptInvocation("2")));
internals.addJavaScriptInvocation(new PendingJavaScriptInvocation(node,
new UIInternals.JavaScriptInvocation("3")));
internals.dumpPendingJavaScriptInvocations();

Mockito.verify(node, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());
}

@Test
public void dumpPendingJavaScriptInvocations_registerOneDetachListenerPerNode() {
StateNode node1 = Mockito.spy(new StateNode(ElementData.class));
node1.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node1);
internals.addJavaScriptInvocation(new PendingJavaScriptInvocation(node1,
new UIInternals.JavaScriptInvocation("1")));

StateNode node2 = Mockito.spy(new StateNode(ElementData.class));
node2.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node2);
internals.addJavaScriptInvocation(new PendingJavaScriptInvocation(node2,
new UIInternals.JavaScriptInvocation("1")));

internals.dumpPendingJavaScriptInvocations();

Mockito.verify(node1, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());
Mockito.verify(node2, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());
}

@Test
public void dumpPendingJavaScriptInvocations_invocationCompletes_pendingListPurged() {
StateNode node = Mockito.spy(new StateNode(ElementData.class));
node.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node);

PendingJavaScriptInvocation invocation = new PendingJavaScriptInvocation(
node, new UIInternals.JavaScriptInvocation(""));
internals.addJavaScriptInvocation(invocation);
internals.dumpPendingJavaScriptInvocations();

Mockito.verify(node, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());

invocation.complete(JsonCodec.encodeWithTypeInfo("OK"));

Assert.assertEquals(0,
internals.getPendingJavaScriptInvocations().count());
}

@Test
public void dumpPendingJavaScriptInvocations_invocationFails_pendingListPurged() {
StateNode node = Mockito.spy(new StateNode(ElementData.class));
node.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node);

PendingJavaScriptInvocation invocation = new PendingJavaScriptInvocation(
node, new UIInternals.JavaScriptInvocation(""));
internals.addJavaScriptInvocation(invocation);
internals.dumpPendingJavaScriptInvocations();

Mockito.verify(node, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());

invocation.completeExceptionally(JsonCodec.encodeWithTypeInfo("ERROR"));

Assert.assertEquals(0,
internals.getPendingJavaScriptInvocations().count());
}

@Test
public void dumpPendingJavaScriptInvocations_invocationCanceled_pendingListPurged() {
StateNode node = Mockito.spy(new StateNode(ElementData.class));
node.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node);

PendingJavaScriptInvocation invocation = new PendingJavaScriptInvocation(
node, new UIInternals.JavaScriptInvocation(""));
internals.addJavaScriptInvocation(invocation);
internals.dumpPendingJavaScriptInvocations();

Mockito.verify(node, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());

invocation.cancelExecution();

Assert.assertEquals(0,
internals.getPendingJavaScriptInvocations().count());
}

@Test
public void dumpPendingJavaScriptInvocations_nodeDetached_pendingListPurged() {
StateNode node = Mockito.spy(new StateNode(ElementData.class));
node.getFeature(ElementData.class).setVisible(false);
internals.getStateTree().getRootNode()
.getFeature(ElementChildrenList.class).add(0, node);

PendingJavaScriptInvocation invocation = new PendingJavaScriptInvocation(
node, new UIInternals.JavaScriptInvocation(""));
internals.addJavaScriptInvocation(invocation);
internals.dumpPendingJavaScriptInvocations();

Mockito.verify(node, Mockito.times(1))
.addDetachListener(ArgumentMatchers.any());

node.setParent(null);

Assert.assertEquals(0,
internals.getPendingJavaScriptInvocations().count());
}

private PushConfiguration setUpInitialPush() {
DeploymentConfiguration config = Mockito
.mock(DeploymentConfiguration.class);
Expand Down

0 comments on commit 38cb83b

Please sign in to comment.