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.