ErrorHandler has no access to UI when command is executed through UI.access() from other thread and throws exception

[ggecy]

Description of the bug / feature

When business logic, which runs in background in separate thread from request-response thread, tries to modify the UI calling ui.access(command) method and the command throws an Exception we have a custom ErrorHandler implementation that tries to display a notification in UI, however there is no access to current UI in the error handler in this case since UI.getCurrent() is populated only during normal code execution and is cleared in com.vaadin.flow.component.UI#accessSynchronously(com.vaadin.flow.server.Command, com.vaadin.flow.function.SerializableRunnable) before calling the error handler in com.vaadin.flow.server.ErrorHandlingCommand#handleError() which is then called without any access to current UI.

Minimal reproducible example

Open UI in browser and then try to run a code from separate thread while there is no request-response running that will throw and Exception within UI.access() command.

Expected behavior

Exception is handled in ErrorHandler implementation registered in session which can modify the UI to show user a notification with useful error message.

Actual behavior

User sees no error message, since error handler has no access to current UI and thus opening notification fails.

Versions:

- Vaadin / Flow version: 2.4.1
- Java version: 11
- OS version: Windows 10
- Browser version (if applicable): Chrome 88
- Application Server (if applicable): Embedded Tomcat in Spring Boot
- IDE (if applicable): Intellij Idea

[pleku]

Hello. We had a discussion whether or not this works as expected, and basically this is one of those things where it "works like coded" which means that while there has not been given any promises that it errors during push/UI.access would go to the UI's error handler, it is quite natural though that the user would expect so. The current behavior has been around probably at least since version 7.

At the moment I'm inclining to calling this a bug, but I don't think I would backport this to a maintenance release since it can change existing application behavior if there are errors happening frequently and it is now intended behavior that those are not shown to the user (it is quite theoretical, but can exist).

[pleku]

The workaround for now would be to catch the exception and showing any kind of error indication, like Notification with error style, to the user. (Now that I write this, I actually think I've done this in a project a very long time ago.)

[ggecy]

Hi, thank you for responding. I am aware I can catch and handle the exception everywhere as workaround, but that is far from ideal., since the goal is to have uniform error notification for all errors and not leave it to individual developers to handle the errors correctly at every place. I have found another workaround - I have static utility method that takes the UI from components and calls UI.setCurrent(ui) before calling the ui.access(command) and then in finally set it back to original if any - this causes the UI#accessSynchronously method to have something it can set back to current instance when it finishes and it seems to be working fine, but developer still needs to not forget to use the utility method everywhere, which is easy to forget and then user has no idea something went wrong since error handler fails to show the notification in that case.

[mvysny]

Good workaround, but far from perfect. Application code shouldn't be forced to call UI.setCurrent(ui) - that's Vaadin's job. Also, there is no thread-safe way to obtain the correct UI from a bg thread:

• Calling someRandomComponent.getUI() is not thread-safe and also will return null if the component is not attached;
• Remembering UI instance in a static field begs for memory leaks and UI overwrite from separate sessions;
• Forcing the developers to call UI.setCurrent(ui) from within ui.access() is error-prone, exactly as you mentioned (programmers may forget to do that).

Vaadin already knows the UI instance, since the error handler is being called by a try{}catch block executed from within the ui.access() function. All Vaadin needs to do is to call UI.setCurrent() before ErrorHandler is invoked, from within FutureAccess. FutureAccess can receive the UI via a constructor.

A workaround would be to extend the UI class and override all access() methods to post a modified Runnable/Command which sets the UI instance into a ThreadLocal, maybe as a WeakReference (so that the UI can be GCed). The ErrorHandler can then retrieve the UI instance from ThreadLocal.

[hellectronic]

Hi, thank you for responding. I am aware I can catch and handle the exception everywhere as workaround, but that is far from ideal., since the goal is to have uniform error notification for all errors and not leave it to individual developers to handle the errors correctly at every place. I have found another workaround - I have static utility method that takes the UI from components and calls UI.setCurrent(ui) before calling the ui.access(command) and then in finally set it back to original if any - this causes the UI#accessSynchronously method to have something it can set back to current instance when it finishes and it seems to be working fine, but developer still needs to not forget to use the utility method everywhere, which is easy to forget and then user has no idea something went wrong since error handler fails to show the notification in that case.

Hello @ggecy,

I have the same problem. We are trying to show an error notification for an exception. We created an ErrorHandler but the UI.getCurrent() ist null (on Vaadin 23).

Do I understand your workaround correctly? You are getting the current UI before doing your specific calls, set it with UI.setCurrent() and set it back in a finally block?

[ggecy]

Hi @hellectronic, here is my GUIUtils implementation - I am using GUIUtils.access() method everywhere instead of UI.access():

import com.vaadin.flow.component.UI;
import com.vaadin.flow.component.Component;
import com.vaadin.flow.function.SerializableConsumer;
import com.vaadin.flow.internal.ExecutionContext;
import com.vaadin.flow.server.Command;
import org.jetbrains.annotations.Nullable;
import org.slf4j.LoggerFactory;

import java.time.Duration;
import java.time.ZoneId;
import java.time.ZoneOffset;
import java.util.function.Consumer;

public final class GUIUtils {

    private GUIUtils() { }

    public static void withZoneOffsetFromBrowser(Consumer<ZoneOffset> consumer) {
        UI.getCurrent().getPage().retrieveExtendedClientDetails(extendedClientDetails -> {
            Duration timezoneOffsetDuration = Duration.ofMillis(extendedClientDetails.getTimezoneOffset());
            consumer.accept(ZoneOffset.ofTotalSeconds((int) timezoneOffsetDuration.getSeconds()));
        });
    }

    public static void withTimeZoneFromBrowser(Consumer<ZoneId> consumer) {
        UI.getCurrent().getPage().retrieveExtendedClientDetails(extendedClientDetails -> consumer.accept(ZoneId.of(extendedClientDetails.getTimeZoneId())));
    }

    public static void access(Component component, Command command) {
        access(component, false, command);
    }

    public static void access(Component component, boolean silent, Command command) {
        var ui = component.getUI().orElse(null);
        var originalUIFromCurrentInstance = UI.getCurrent();

        // UI.getCurrent() might be null in async calls, this causes problems for exception handling since there is no
        // access to ui except UI.getCurrent()
        if (ui != null && !ui.equals(originalUIFromCurrentInstance)) {
            // if we have UI in component make sure it is available through UI.getCurrent()
            UI.setCurrent(ui);
        } else if (ui == null && originalUIFromCurrentInstance != null) {
            // if we have UI.getCurrent() but component is not attached yet, use UI.getCurrent()
            ui = originalUIFromCurrentInstance;
        }

        try {
            if (ui != null) {
                ui.access(command);
            } else if (!silent) {
                throw new GUIRuntimeException("No UI available, cannot execute command");
            } else {
                LoggerFactory.getLogger(component.getClass()).warn("Cannot run command, no UI available to access");
            }
        } finally {
            if (ui != null && !ui.equals(originalUIFromCurrentInstance)) {
                UI.setCurrent(originalUIFromCurrentInstance);
            }
        }
    }

    public static void runWhenAttachedBeforeClientResponse(Component component, SerializableConsumer<ExecutionContext> execution) {
        component.getElement().getNode().runWhenAttached(ui -> ui.beforeClientResponse(component, execution));
    }

    public static void handleInUIErrorHandler(Component component, Throwable throwable, String message) {
        handleInUIErrorHandler(component, throwable, message, null);
    }

    public static void handleInUIErrorHandler(Component component, Throwable throwable, String message, @Nullable Runnable doOnError) {
        access(component, () -> {
            if (doOnError != null) {
                doOnError.run();
            }
            throw new GUIRuntimeException(message, throwable);
        });
    }

}

[hellectronic]

Hello @ggecy ,

thanks a lot!

[oliveryasuna]

Thanks to @rodolforfq for referring me to this issue.

I ran into a similar case when I threw NotFoundException from a background thread. I expected Vaadin to show my HasErrorParameter<NotFoundException> view. Rather, Vaadin does not handle exceptions in the Future and a ExecutionException is thrown. I do not consider this a bug because checking for exceptions is part of the navigation cycle of JavaScriptBootstrapUI.

This is my workaround:

public final class CustomErrorHandler extends DefaultErrorHandler {

  public CustomErrorHandler() {
    super();
  }

  @Override
  public final void error(final ErrorEvent event) {
    final Throwable throwable = event.getThrowable();

    if(throwable instanceof final ExecutionException executionException) {
      final Throwable cause = executionException.getCause();

      // May have a error target entry.
      if(cause instanceof final Exception exception) {
        // The exception may have been thrown in a `UI#access` block.

        // Get the UI and router.
        final UI ui = UI.getCurrent();
        final Router router = ui.getInternals().getRouter();

        // Get the error target entry.
        final Optional<ErrorTargetEntry> optionalErrorTargetEntry = router.getErrorNavigationTarget(exception);

        if(optionalErrorTargetEntry.isPresent()) {
          final ErrorTargetEntry errorTargetEntry = optionalErrorTargetEntry.get();

          final ErrorParameter<?> errorParameter = new ErrorParameter<>(errorTargetEntry.getHandledExceptionType(), exception, exception.getMessage());
          final ErrorStateRenderer errorStateRenderer = new ErrorStateRenderer(new NavigationStateBuilder(router)
              .withTarget(errorTargetEntry.getNavigationTarget())
              .build());

          ui.getPage().fetchCurrentURL(url -> {
            final String location = url.getPath().substring(VaadinRequest.getCurrent().getContextPath().length() + 1);

            final ErrorNavigationEvent errorNavigationEvent = new ErrorNavigationEvent(router, new Location(location), ui, NavigationTrigger.PROGRAMMATIC, errorParameter);

            errorStateRenderer.handle(errorNavigationEvent);
          });

          return;
        }
      }
    }

    // Call the default error handler.
    super.error(event);
  }

}

This is definitely not an ideal solution as it uses Vaadin internal classes and I do not consider it production-ready. Also, it could be called after the view is rendered. Use with care :)

[mvysny]

I think this is a duplicite of https://github.com/vaadin/flow/issues/10533 which has been implemented and closed already.