Prefer accessing UI through attached elements over UI.getCurrent()

[vursen]

Describe your motivation

Components sometimes need access to the UI instance, but there isn't a single standard way how to get that instance. One of the approaches widely used across the codebase is UI.getCurrent(). However, it's often neglected that this API isn't thread-safe and may return null in some contexts. One such example is when the component instance is created in a background thread without a UI lock:

@Route("grid")
public class GridView extends Div {
    public GridView() {
        Button addGrid = new Button("Add grid", (event) -> {
            final VaadinSession session = VaadinSession.getCurrent();

            new Thread(() -> session.accessSynchronously(this::render)).start();
        });

        add(addGrid);
    }

    public void render() {
        // UI isn't available in this context
        Grid<String> grid = new Grid<>();
        // ComponentRenderer is a class that relies on UI.getCurrent()
        grid.addColumn(new ComponentRenderer<Span, String>(item -> new Span(item)));
        grid.setItems("Item 1", "Item 2", "Item 3", "Item 4", "Item 5");
        add(grid);
    }
}

Then, in ComponentRenderer, a variable is set to an empty string if UI.getCurrent() is null. This prevents null pointer exceptions but still eventually causes the app to break elsewhere, which isn't the best solution:

https://github.com/vaadin/flow-components/blob/7249a90e014dc9d544b2cda531169c6e13255d5e/vaadin-renderer-flow-parent/vaadin-renderer-flow/src/main/java/com/vaadin/flow/data/renderer/ComponentRenderer.java#L146-L149

Here are similar examples from other modules:

https://github.com/vaadin/flow-components/blob/7249a90e014dc9d544b2cda531169c6e13255d5e/vaadin-renderer-flow-parent/vaadin-renderer-flow/src/main/java/com/vaadin/flow/data/renderer/LitRenderer.java#L83-L90

https://github.com/vaadin/flow-components/blob/7249a90e014dc9d544b2cda531169c6e13255d5e/vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/Spreadsheet.java#L1181-L1184

https://github.com/vaadin/flow-components/blob/7249a90e014dc9d544b2cda531169c6e13255d5e/vaadin-charts-flow-parent/vaadin-charts-flow/src/main/java/com/vaadin/flow/component/charts/ChartOptions.java#L48-L57

https://github.com/vaadin/flow-components/blob/7249a90e014dc9d544b2cda531169c6e13255d5e/vaadin-dialog-flow-parent/vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java#L1052-L1056

Describe the solution you'd like

Instead of using UI.getCurrent(), methods should access UI through an attached component or element that is available in the current context. This is a thread-safe approach. Here are some ways to do this:

if (isAttached()) {
  UI ui = getUI();

  // do something with UI from the component
}

getElement().getNode().runWhenAttached(ui -> {
  // do something with UI once for this element
}); 

element.addAttachListener(event -> {
  // attach event doesn't provide a UI reference for elements
  UI ui = ((StateTree) element.getNode().getOwner()).getUI();

  // do something with UI on every attach of this element
});

if (element.isAttached()) {
  UI ui = ((StateTree) element.getNode().getOwner()).getUI();

  // do something with UI if this element is already attached
}

However, if no attached element is available and hence the use of UI.getCurrent() is unavoidable, the method documentation should state this requirement and the null case should be handled properly with a clear exception like here:

https://github.com/vaadin/flow-components/blob/7249a90e014dc9d544b2cda531169c6e13255d5e/vaadin-dialog-flow-parent/vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java#L762-L772

Additional context

Originally discovered by @archiecobbs in https://github.com/vaadin/flow/issues/20240#issuecomment-2410952585

Related #6751

[vursen]

Similar issue for LitRenderer specifically: https://github.com/vaadin/flow-components/issues/4963 (also caused by UI.getCurrent)

[vursen]

In our internal discussion, it was suggested that we could improve the DX for existing UI.getCurrent instances if a UI.getCurrentOrThrow API was introduced on the Flow side. This alternative to UI.getCurrent would require minimal refactoring and would at least provide a clear uniform exception explaining that a UI instance is required.

@mshabarov What do you think? Would it be possible to add this API?

[mstahv]

Store the UI reference (and expose it) in ComponentEvent. I think I would never use a method like UI.getCurrentOrThrow (would prefer to make a null check for UI.getCurrent()).

The best option would be that Component.getUI would be thread safe. The current situation is quite odd (that it is not) and it is not even documented 🤔

[mshabarov]

An Optional version of UI.getCurrent makes sense for the public Flow API, something like public static Optional<UI> get() in the UI class, that method can also give hints what to do if the result is empty.

But not sure the one with throwing an exception is a good solution long-term, I’d avoid it. Better to make a helper and disclaim that it’s an internal API.

[TatuLund]

The best option would be that Component.getUI would be thread safe. The current situation is quite odd (that it is not) and it is not even documented

The Vaadin way documentation should cover this now actually. We had discussion about it. The weird part of getUI() is that if you have a composition that has not been used yet, getUI will trigger to initialize it. This is the most thread unsafe part of it.

IMHO, those components that do currently need to use access(..), are more complex and could just take the ui instance on attach and work that way. I had an idea to include UI access delegate method in Component, but that idea was rejected as it adds a bit more weight to all components, including those 95%+ where you do not need it https://github.com/vaadin/flow/pull/20105 When Signals library is ready, it will actually remove lot of need for using UI.access in application code, so from that perspective also delegate method in the component will become un-necessary.

[archiecobbs]

Hi @Artur,

Just want to make sure I understand what's been fixed and what hasn't here.

It seems that that all that #8241 does is convert NullPointerExceptions into IllegalStateExceptions - correct?

If so, what is the final decision as how to "Prefer accessing UI through attached elements over UI.getCurrent()"?

Has Vaadin eliminated all points in the code where it is being assumed that just because there is a current VaadinSession, there must also be a current UI?

Thanks for any clarification.

[Artur-]

The only change in #8241 is that it throws a more descriptive exception in the cases where there is not a current UI. It does not solve the problem that some APIs should be redesigned so that they do not rely on there being a current UI instance

[archiecobbs]

The only change in #8241 is that it throws a more descriptive exception in the cases where there is not a current UI.

Thanks for the clarification.

It does not solve the problem that some APIs should be redesigned so that they do not rely on there being a current UI instance

So what github issue tracks that problem now?

[vursen]

So what github issue tracks that problem now?

The examples listed in this ticket, except for LitRenderer and ComponentRenderer, require a UI instance by design, so the issue was addressed by throwing a more descriptive exception. LitRenderer and ComponentRenderer, on the other hand, required a UI instance by mistake, which was corrected in the following PRs:

• https://github.com/vaadin/flow-components/pull/6759
• https://github.com/vaadin/flow-components/pull/6761

If you have other examples like LitRenderer, feel free to reopen the ticket.

[archiecobbs]

@vursen - OK great, thanks! Just wanted to double check the other known problems were being addressed.

[vursen]

It turns out there are still a few places where the use of UI.getCurrent could be avoided:

1. https://github.com/vaadin/flow-components/blob/34dce396c690793172df3136ca422483eee4774c/vaadin-dialog-flow-parent/vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java#L1233-L1234
2. https://github.com/vaadin/flow-components/blob/34dce396c690793172df3136ca422483eee4774c/vaadin-dashboard-flow-parent/vaadin-dashboard-flow/src/main/java/com/vaadin/flow/component/dashboard/Dashboard.java#L613-L614
3. https://github.com/vaadin/flow-components/blob/34dce396c690793172df3136ca422483eee4774c/vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/PopupButton.java#L129-L132
4. https://github.com/vaadin/flow-components/blob/34dce396c690793172df3136ca422483eee4774c/vaadin-flow-components-shared-parent/vaadin-flow-components-base/src/main/java/com/vaadin/flow/component/shared/Tooltip.java#L109-L110

I'm reopening the ticket. @archiecobbs Thank you for bringing this to our attention.