Suppress unavoidable UIDetachedException (#11146)

* Suppress unavoidable UIDetachedException

Fixes #11144

server/src/main/java/com/vaadin/server/ErrorHandlingRunnable.java

@@ -16,6 +16,7 @@
 package com.vaadin.server;
 
 import java.io.Serializable;
+import java.util.Objects;
 
 /**
  * Defines the interface to handle exceptions thrown during the execution of a
@@ -28,6 +29,8 @@ public interface ErrorHandlingRunnable extends Runnable, Serializable {
 
     /**
      * Handles exceptions thrown during the execution of a FutureAccess.
+     * Exceptions thrown by this method are handled by the default error
+     * handler.
      *
      * @since 7.1.8
      * @param exception
@@ -35,4 +38,38 @@ public interface ErrorHandlingRunnable extends Runnable, Serializable {
      */
     public void handleError(Exception exception);
 
+    /**
+     * Process the given exception in the context of the given runnable. If the
+     * runnable extends {@link ErrorHandlingRunnable}, then the exception is
+     * passed to {@link #handleError(Exception)} and null is returned. If
+     * {@link #handleError(Exception)} throws an exception, that exception is
+     * returned. If the runnable does not extend {@link ErrorHandlingRunnable},
+     * then the original exception is returned.
+     *
+     * @since 
+     * @param runnable
+     *            the runnable for which the exception should be processed, not
+     *            <code>null</code>
+     * @param exception
+     *            the exception to process, not <code>null</code>
+     * @return the resulting exception, or <code>null</code> if the exception is
+     *         fully processed
+     */
+    public static Exception processException(Runnable runnable,
+            Exception exception) {
+        Objects.requireNonNull(runnable, "The runnable cannot be null.");
+        if (runnable instanceof ErrorHandlingRunnable) {
+            ErrorHandlingRunnable errorHandlingRunnable = (ErrorHandlingRunnable) runnable;
+
+            try {
+                errorHandlingRunnable.handleError(exception);
+                return null;
+            } catch (Exception exceptionFromHandler) {
+                return exceptionFromHandler;
+            }
+        }
+
+        return exception;
+    }
+
 }

server/src/main/java/com/vaadin/server/VaadinService.java

@@ -40,6 +40,7 @@ import java.util.ServiceLoader;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.CopyOnWriteArrayList;
+import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.locks.Lock;
@@ -2047,8 +2048,13 @@ public abstract class VaadinService implements Serializable {
 
                     try {
                         pendingAccess.get();
-
                     } catch (Exception exception) {
+                        if (exception instanceof ExecutionException) {
+                            Throwable cause = exception.getCause();
+                            if (cause instanceof Exception) {
+                                exception = (Exception) cause;
+                            }
+                        }
                         pendingAccess.handleError(exception);
                     }
                 }

server/src/main/java/com/vaadin/server/VaadinSession.java

@@ -123,11 +123,10 @@ public class VaadinSession implements HttpSessionBindingListener, Serializable {
          */
         public void handleError(Exception exception) {
             try {
-                if (runnable instanceof ErrorHandlingRunnable) {
-                    ErrorHandlingRunnable errorHandlingRunnable = (ErrorHandlingRunnable) runnable;
+                exception = ErrorHandlingRunnable.processException(runnable,
+                        exception);
 
-                    errorHandlingRunnable.handleError(exception);
-                } else {
+                if (exception != null) {
                     ErrorEvent errorEvent = new ErrorEvent(exception);
 
                     ErrorHandler errorHandler = ErrorEvent

server/src/main/java/com/vaadin/ui/UI.java

@@ -230,8 +230,8 @@ public abstract class UI extends AbstractSingleComponentContainer
             }
             json.append("]}");
             getRpcProxy(DebugWindowClientRpc.class)
-                    .reportLayoutProblems(json.toString());
-        }
+                            .reportLayoutProblems(json.toString());
+}
 
         @Override
         public void showServerDesign(Connector connector) {
@@ -1567,17 +1567,45 @@ public abstract class UI extends AbstractSingleComponentContainer
             @Override
             public void handleError(Exception exception) {
                 try {
-                    if (runnable instanceof ErrorHandlingRunnable) {
-                        ErrorHandlingRunnable errorHandlingRunnable = (ErrorHandlingRunnable) runnable;
-
-                        errorHandlingRunnable.handleError(exception);
-                    } else {
+                    exception = ErrorHandlingRunnable.processException(runnable,
+                            exception);
+
+                    if (exception instanceof UIDetachedException) {
+                        assert session != null;
+                        /*
+                         * UI was detached after access was run, but before
+                         * accessSynchronously. Furthermore, there wasn't an
+                         * ErrorHandlingRunnable that handled the exception.
+                         */
+                        getLogger().log(Level.WARNING,
+                                "access() task ignored because UI got detached after the task was enqueued."
+                                        + " To suppress this message, change the task to implement {} and make it handle {}."
+                                        + " Affected task: {}",
+                                        new Object[] {
+                                                ErrorHandlingRunnable.class.getName(),
+                                                UIDetachedException.class.getName(),
+                                                runnable });
+                    } else if (exception != null) {
+                        /*
+                         * If no ErrorHandlingRunnable, or if it threw an
+                         * exception of its own.
+                         */
                         ConnectorErrorEvent errorEvent = new ConnectorErrorEvent(
                                 UI.this, exception);
 
                         ErrorHandler errorHandler = com.vaadin.server.ErrorEvent
                                 .findErrorHandler(UI.this);
 
+                        if (errorHandler == null && getSession() == null) {
+                            /*
+                             * Special case where findErrorHandler(UI) cannot
+                             * find the session handler because the UI has
+                             * recently been detached.
+                             */
+                            errorHandler = com.vaadin.server.ErrorEvent
+                                    .findErrorHandler(session);
+                        }
+
                         if (errorHandler == null) {
                             errorHandler = new DefaultErrorHandler();
                         }
@@ -1699,7 +1727,7 @@ public abstract class UI extends AbstractSingleComponentContainer
         // If pushMode is disabled then there should never be a pushConnection;
         // if enabled there should always be
         assert (pushConnection == null)
-                ^ getPushConfiguration().getPushMode().isEnabled();
+        ^ getPushConfiguration().getPushMode().isEnabled();
 
         if (pushConnection == this.pushConnection) {
             return;

uitest/src/main/java/com/vaadin/tests/components/AbstractTestUI.java

@@ -177,6 +177,11 @@ public abstract class AbstractTestUI extends UI {
         getLayout().addComponent(c);
     }
 
+    public void addComponent(Component c, String id) {
+        c.setId(id);
+        addComponent(c);
+    }
+
     public void addComponents(Component... c) {
         getLayout().addComponents(c);
     }

uitest/src/main/java/com/vaadin/tests/components/ui/DetachedAccessErrorHandling.java

@@ -0,0 +1,78 @@
+package com.vaadin.tests.components.ui;
+
+import java.util.List;
+import java.util.concurrent.CopyOnWriteArrayList;
+
+import com.vaadin.annotations.Widgetset;
+import com.vaadin.server.Constants;
+import com.vaadin.server.ErrorHandler;
+import com.vaadin.server.ErrorHandlingRunnable;
+import com.vaadin.server.VaadinRequest;
+import com.vaadin.tests.components.AbstractTestUI;
+import com.vaadin.ui.Button;
+import com.vaadin.ui.Label;
+import com.vaadin.ui.UIDetachedException;
+
+@Widgetset(Constants.DEFAULT_WIDGETSET)
+public class DetachedAccessErrorHandling extends AbstractTestUI {
+
+    private static final Runnable NOP = () -> {
+    };
+
+    private static final class ListErrorHandler implements ErrorHandler {
+        private List<com.vaadin.server.ErrorEvent> errors = new CopyOnWriteArrayList<>();
+
+        @Override
+        public void error(com.vaadin.server.ErrorEvent event) {
+            errors.add(event);
+        }
+    }
+
+    @Override
+    protected void setup(VaadinRequest request) {
+        ListErrorHandler errorHandler = ensureErrorHandlerSet();
+
+        addComponent(new Button("Show errors", event -> {
+            errorHandler.errors.forEach(error -> {
+                Label errorLabel = new Label(error.getThrowable().getMessage());
+                errorLabel.setStyleName("errorLabel");
+                addComponent(errorLabel);
+            });
+        }), "show");
+
+        addComponent(new Button("Add simple detach listener", event -> {
+            addDetachListener(detachEvent -> access(NOP));
+        }), "simple");
+
+        addComponent(new Button("Add error handling detach listener", event -> {
+            addDetachListener(detachEvent -> {
+                access(new ErrorHandlingRunnable() {
+                    @Override
+                    public void run() {
+                    }
+
+                    @Override
+                    public void handleError(Exception exception) {
+                        if (exception instanceof UIDetachedException) {
+                            UIDetachedException ignore = (UIDetachedException) exception;
+                        } else {
+                            throw new RuntimeException(exception);
+                        }
+                    }
+                });
+            });
+        }), "handling");
+    }
+
+    private ListErrorHandler ensureErrorHandlerSet() {
+        ErrorHandler currentErrorHandler = getSession().getErrorHandler();
+
+        if (!(currentErrorHandler instanceof ListErrorHandler)) {
+            currentErrorHandler = new ListErrorHandler();
+            getSession().setErrorHandler(currentErrorHandler);
+        }
+
+        return (ListErrorHandler) currentErrorHandler;
+    }
+
+}

uitest/src/main/java/com/vaadin/tests/components/ui/UIAccessExceptionHandling.java

@@ -6,6 +6,7 @@ import java.util.concurrent.Future;
 
 import com.vaadin.server.DefaultErrorHandler;
 import com.vaadin.server.ErrorHandler;
+import com.vaadin.server.ErrorHandlingRunnable;
 import com.vaadin.server.VaadinRequest;
 import com.vaadin.server.VaadinService;
 import com.vaadin.tests.components.AbstractTestUIWithLog;
@@ -70,6 +71,27 @@ public class UIAccessExceptionHandling extends AbstractTestUIWithLog
                     CurrentInstance.restoreInstances(instances);
                 }));
 
+        addComponent(
+                new Button("Throw through ErrorHandlingRunnable", event -> {
+                    access(new ErrorHandlingRunnable() {
+                        @Override
+                        public void run() {
+                            log.clear();
+                            throw new NullPointerException();
+                        }
+
+                        @Override
+                        public void handleError(Exception exception) {
+                            // "Handle" other exceptions, but leave NPE for
+                            // default handler
+                            if (exception instanceof NullPointerException) {
+                                NullPointerException npe = (NullPointerException) exception;
+                                throw npe;
+                            }
+                        }
+                    });
+                }));
+
         addComponent(new Button("Clear", event -> log.clear()));
     }
 

uitest/src/test/java/com/vaadin/tests/components/ui/DetachedAccessErrorHandlingTest.java

@@ -0,0 +1,31 @@
+package com.vaadin.tests.components.ui;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.openqa.selenium.By;
+
+import com.vaadin.testbench.elements.ButtonElement;
+import com.vaadin.tests.tb3.SingleBrowserTest;
+
+public class DetachedAccessErrorHandlingTest extends SingleBrowserTest {
+    @Test
+    public void testDetachedErrorHandling_pageOpen_noErrors() {
+        openTestURL();
+
+        $(ButtonElement.class).id("simple").click();
+        assertNoErrors();
+
+        // The thing to really test here is that nothing is logged to stderr,
+        // but that's not practical to detect
+        $(ButtonElement.class).id("handling").click();
+        assertNoErrors();
+    }
+
+    private void assertNoErrors() {
+        // Reload page to trigger detach event
+        openTestURL();
+
+        $(ButtonElement.class).id("show").click();
+        Assert.assertEquals(0, findElements(By.className("errorLabel")).size());
+    }
+}

uitest/src/test/java/com/vaadin/tests/components/ui/UIAccessExceptionHandlingTest.java

@@ -5,9 +5,9 @@ import static org.junit.Assert.assertEquals;
 import org.junit.Test;
 
 import com.vaadin.testbench.elements.ButtonElement;
-import com.vaadin.tests.tb3.MultiBrowserTest;
+import com.vaadin.tests.tb3.SingleBrowserTest;
 
-public class UIAccessExceptionHandlingTest extends MultiBrowserTest {
+public class UIAccessExceptionHandlingTest extends SingleBrowserTest {
 
     @Test
     public void testExceptionHandlingOnUIAccess() throws Exception {
@@ -15,17 +15,21 @@ public class UIAccessExceptionHandlingTest extends MultiBrowserTest {
         $(ButtonElement.class).first().click();
         assertLogTexts(
                 "1. Exception caught on get: java.util.concurrent.ExecutionException",
-                "0. Exception caught on execution with ConnectorErrorEvent : java.util.concurrent.ExecutionException");
+                "0. Exception caught on execution with ConnectorErrorEvent : java.lang.RuntimeException");
 
         $(ButtonElement.class).get(1).click();
         assertLogTexts(
                 "1. Exception caught on get: java.util.concurrent.ExecutionException",
-                "0. Exception caught on execution with ErrorEvent : java.util.concurrent.ExecutionException");
+                "0. Exception caught on execution with ErrorEvent : java.lang.RuntimeException");
 
         $(ButtonElement.class).get(2).click();
         assertLogTexts(
                 "1. Exception caught on get: java.util.concurrent.ExecutionException",
-                "0. Exception caught on execution with ConnectorErrorEvent : java.util.concurrent.ExecutionException");
+                "0. Exception caught on execution with ConnectorErrorEvent : java.lang.RuntimeException");
+
+        $(ButtonElement.class).get(3).click();
+        assertLogText(0,
+                "0. Exception caught on execution with ConnectorErrorEvent : java.lang.NullPointerException");
     }
 
     private void assertLogTexts(String first, String second) {