UI.getCurrent() is null in ErrorHandler

[mvysny]

Throwing an exception in the ui.access() block from bg thread will call ErrorHandler.error() with UI.getCurrent() set to null. That will fail to show any notification from the ErrorHandler with the following stacktrace:

[Thread-8] ERROR com.vaadin.flow.component.UI - UI instance is not available. It means that you are calling this method out of a normal workflow where it's always implicitly set. That may happen if you call the method from the custom thread without 'UI::access' or from tests without proper initialization.
java.lang.IllegalStateException: UI instance is not available. It means that you are calling this method out of a normal workflow where it's always implicitly set. That may happen if you call the method from the custom thread without 'UI::access' or from tests without proper initialization.
	at com.vaadin.flow.component.notification.Notification.setOpened(Notification.java:426)
	at com.vaadin.flow.component.notification.Notification.open(Notification.java:303)
	at com.vaadin.flow.component.notification.Notification.show(Notification.java:227)
	at com.vaadin.flow.component.notification.Notification.show(Notification.java:244)
	at org.vaadin.example.Bootstrap.error(Bootstrap.java:20)
	at com.vaadin.flow.component.UI$1.handleError(UI.java:520)
	at com.vaadin.flow.server.FutureAccess.handleError(FutureAccess.java:76)
	at com.vaadin.flow.server.VaadinService.runPendingAccessTasks(VaadinService.java:2045)
	at com.vaadin.flow.server.VaadinSession.unlock(VaadinSession.java:681)
	at com.vaadin.flow.server.VaadinService.ensureAccessQueuePurged(VaadinService.java:2005)
	at com.vaadin.flow.server.VaadinService.accessSession(VaadinService.java:1972)
	at com.vaadin.flow.server.VaadinSession.access(VaadinSession.java:969)
	at com.vaadin.flow.component.UI.access(UI.java:506)
	at com.vaadin.flow.component.UI.access(UI.java:489)
	at org.vaadin.example.MainView$1.run(MainView.java:52)

The ErrorHandler does not document whether it's called in Vaadin UI thread or not. If it doesn't, this needs to be clearly documented. If yes, then the Vaadin environment should be set properly - the UI.getCurrent() should not return null. In either case, this needs to be documented in the ErrorHandler javadoc.

Minimal reproducible example

Attaching the skeleton starter app which demoes the issue. In short, this view demoes the issue:

public class MainView extends VerticalLayout {

    public MainView() {
        // Use TextField for standard text input
        TextField textField = new TextField("Wait for it");
        add(textField);

        final UI ui = UI.getCurrent();
        new Thread() {
            @Override
            public void run() {
                try {
                    Thread.sleep(1000);
                    ui.access(() -> {
                       throw new RuntimeException("Simulated failure");
                    });
                } catch (Throwable t) {
                    t.printStackTrace();
                }
            }
        }.start();
    }
}

Make sure to have the ErrorHandler set; wait for 1 second then check out the console log.

skeleton-starter-flow.zip

Expected behavior

The UI.getCurrent() should not be null.

Actual behavior

The UI.getCurrent() is null.

Versions:

- Vaadin / Flow version: 14.5.1 / 2.5.1
- Java version: 11

[knoobie]

Reminds me of #9971

[TatuLund]

This is not a bug, it just means that you need to setup your ErrorHandler to be thread aware, e.g. this worked for me

public class InitListener implements VaadinServiceInitListener {

    @Override
    public void serviceInit(ServiceInitEvent serviceInitEvent) {
        serviceInitEvent.getSource().addUIInitListener(event -> {
            UI ui = event.getUI();
            VaadinSession session = ui.getSession();
            setErrorHandler(session, ui);
        }
    }

    private void setErrorHandler(VaadinSession session, UI ui) {
        session.setErrorHandler(error -> {
            ui.access(() -> {
                ConfirmDialog confirmDialog = new ConfirmDialog("Error",
                        "Internal Error: " + error.getThrowable().getMessage(),
                        "Do Something", confirmFire -> {
                        });
                String trace = "";
                for (StackTraceElement s : error.getThrowable()
                        .getStackTrace()) {
                    trace = trace + "  at " + s.getClassName() + ".java line "
                            + s.getLineNumber() + " " + s.getMethodName()
                            + "\n";
                }
                confirmDialog.add(new Pre(trace));
                confirmDialog.setWidth("500px");
                confirmDialog.setHeight("500px");
                confirmDialog.open();
                error.getThrowable().printStackTrace();
            });
        });
    }
}

[mvysny]

I disagree. In my eyes I'd expect error handler to run in the ui thread since it will most probably notify the user of the error via some ui call. It does not which is unexpected, not clarified via a documentation and therefore a bug.

The current solution's behavior is mysterious:

• It sets the current session, but not the current UI, for no apparent reason. No explanation is given in javadoc.
• However, if the ErrorHandler is called from Vaadin UI thread (e.g. when a button click listener throws an exception) then both the current UI and current session is set.
• Is there a reason for this duality? Hopefully something better than "we developed it that way, therefore it's correct"
• If this is not a bug, then it's a very bad design.

The example code is broken for scenario when you have more than one UI (unlikely but possible). In such case, your code will notify just one random ui, since the error handler is session-scoped and the UI init listener will overwrite the error handler set by the previous UI init listener.

[pleku]

This is exactly a duplicate of #9971 which is marked as a bug as I would not claim that "the way it is coded is good enough" but this issue can be about fixing the documentation+javadocs and the other issue about fixing the problem by changing the behavior.

And I think the the workaround in #10533 (comment) is bad but #9971 (comment) should work instead.

[mvysny]

but #9971 (comment) should work instead.

It doesn't if there are multiple UI instances (multiple tabs). Regardless, it's Vaadin's job to properly fill in the current UI.

[TatuLund]

Regardless, it's Vaadin's job to properly fill in the current UI.

I would say that current API is a bit bad with that respect. One approach to make API better would be to move setErrorHandler in UI init handler instead of setting it for service. That would need some refactorization and naturally would be breaking change ... Thus I guess requires careful design. Also one should decide what to do with old setErrorHandler method. Whether it should be just marked depracted with notes why, or should it be rendered to no operation state.

[mvysny]

One approach to make API better would be to move setErrorHandler in UI init handler instead of setting it for service.

No need - far simpler is to make Vaadin to fill in the current UI correctly. Vaadin KNOWS the UI, it's just not passing it in.

[TatuLund]

Vaadin KNOWS the UI, it's just not passing it in.

It is not that simple. Sevice - UI is one to many mapping when there are multiple browser tabs.

[mvysny]

Sevice - UI is one to many mapping when there are multiple browser tabs.

That is true, but Vaadin can simply remember the UI instance in the ui.access() itself, for example within the new ErrorHandlingCommand() when it is constructed. Or in the FutureAccess class. If there is a will, there is a way ;)