fix: catch exceptions from detach calls (#20656)

* fix: catch exceptions from detach calls * add test --------- Co-authored-by: Marco Collovati <marco@vaadin.com>
vaadin · Dec 13, 2024 · a2da336 · a2da336
1 parent c06a0a3
commit a2da336
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 2 deletions.
diff --git a/flow-server/src/main/java/com/vaadin/flow/component/ComponentUtil.java b/flow-server/src/main/java/com/vaadin/flow/component/ComponentUtil.java
@@ -48,6 +48,7 @@
 import com.vaadin.flow.router.Route;
 import com.vaadin.flow.router.Router;
 import com.vaadin.flow.server.Attributes;
+import com.vaadin.flow.server.ErrorEvent;
 import com.vaadin.flow.server.VaadinService;
 import com.vaadin.flow.server.VaadinSession;
 import com.vaadin.flow.shared.Registration;
@@ -338,7 +339,12 @@ public static void onComponentDetach(Component component) {
         }
 
         DetachEvent detachEvent = new DetachEvent(component);
-        component.onDetach(detachEvent);
+        try {
+            component.onDetach(detachEvent);
+        } catch (RuntimeException e) {
+            VaadinSession.getCurrent().getErrorHandler()
+                    .error(new ErrorEvent(e));
+        }
         fireEvent(component, detachEvent);
 
         // inform component about onEnabledState if parent and child states

diff --git a/flow-server/src/main/java/com/vaadin/flow/component/DetachNotifier.java b/flow-server/src/main/java/com/vaadin/flow/component/DetachNotifier.java
@@ -17,6 +17,8 @@
 
 import java.io.Serializable;
 
+import com.vaadin.flow.server.ErrorEvent;
+import com.vaadin.flow.server.VaadinSession;
 import com.vaadin.flow.shared.Registration;
 
 /**
@@ -38,7 +40,14 @@ default Registration addDetachListener(
             ComponentEventListener<DetachEvent> listener) {
         if (this instanceof Component) {
             return ComponentUtil.addListener((Component) this,
-                    DetachEvent.class, listener);
+                    DetachEvent.class, event -> {
+                        try {
+                            listener.onComponentEvent(event);
+                        } catch (RuntimeException e) {
+                            VaadinSession.getCurrent().getErrorHandler()
+                                    .error(new ErrorEvent(e));
+                        }
+                    });
         } else {
             throw new IllegalStateException(String.format(
                     "The class '%s' doesn't extend '%s'. "

diff --git a/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java b/flow-server/src/test/java/com/vaadin/flow/component/ComponentTest.java
@@ -18,6 +18,7 @@
 import java.lang.reflect.Field;
 import java.util.ArrayList;
 import java.util.Collection;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -737,6 +738,76 @@ public void testDetachListener_eventOrder_childFirst() {
         parent.assertDetachEvents(1);
     }
 
+    @Test
+    public void testDetach_failingListeners_allListenersInvokedAndExceptionHandled() {
+        Set<Throwable> expectedExceptions = new HashSet<>();
+        Set<Throwable> handledExceptions = new HashSet<>();
+        VaadinSession session = new AlwaysLockedVaadinSession(
+                new MockVaadinServletService());
+        session.setErrorHandler(
+                event -> handledExceptions.add(event.getThrowable()));
+        VaadinSession.setCurrent(session);
+        try {
+            UI ui = new UI();
+            TestComponentContainer parent = new TestComponentContainer() {
+                @Override
+                protected void onDetach(DetachEvent detachEvent) {
+                    getDetachEvents().incrementAndGet();
+                    UnsupportedOperationException exception = new UnsupportedOperationException(
+                            "ON-DETACH-1");
+                    expectedExceptions.add(exception);
+                    throw exception;
+                }
+            };
+            TestComponent child = new TestComponent() {
+                @Override
+                protected void onDetach(DetachEvent detachEvent) {
+                    getDetachEvents().incrementAndGet();
+                    UnsupportedOperationException exception = new UnsupportedOperationException(
+                            "ON-DETACH-2");
+                    expectedExceptions.add(exception);
+                    throw exception;
+                }
+            };
+            child.track();
+            parent.track();
+
+            child.addDetachListener(event -> {
+                if (event.getSource() instanceof TracksAttachDetach track) {
+                    track.getDetachEvents().incrementAndGet();
+                }
+                UnsupportedOperationException exception = new UnsupportedOperationException(
+                        "DETACH-LISTENER-1");
+                expectedExceptions.add(exception);
+                throw exception;
+            });
+            parent.addDetachListener(event -> {
+                if (event.getSource() instanceof TracksAttachDetach track) {
+                    track.getDetachEvents().incrementAndGet();
+                }
+                UnsupportedOperationException exception = new UnsupportedOperationException(
+                        "DETACH-LISTENER-2");
+                expectedExceptions.add(exception);
+                throw exception;
+            });
+
+            parent.add(child);
+            ui.add(parent);
+
+            child.assertDetachEvents(0);
+            parent.assertDetachEvents(0);
+
+            ui.remove(parent);
+
+            child.assertDetachEvents(3);
+            parent.assertDetachEvents(3);
+
+            Assert.assertEquals(expectedExceptions, handledExceptions);
+        } finally {
+            VaadinSession.setCurrent(null);
+        }
+    }
+
     @Test
     public void testAttachDetach_elementMoved_bothEventsTriggered() {
         UI ui = new UI();