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:

flow-components/vaadin-renderer-flow-parent/vaadin-renderer-flow/src/main/java/com/vaadin/flow/data/renderer/ComponentRenderer.java

Lines 146 to 149 in 7249a90

	protected String getTemplateExpression() {
	var appId = UI.getCurrent() != null
	? UI.getCurrent().getInternals().getAppId()
	: "";

Here are similar examples from other modules:

flow-components/vaadin-renderer-flow-parent/vaadin-renderer-flow/src/main/java/com/vaadin/flow/data/renderer/LitRenderer.java

Lines 83 to 90 in 7249a90

	if (UI.getCurrent() != null) {
	// Generate a unique (in scope of the UI) namespace for the renderer
	// properties.
	litRendererCount = UI.getCurrent().getElement()
	.getProperty("__litRendererCount", 0);
	UI.getCurrent().getElement().setProperty("__litRendererCount",
	litRendererCount + 1);

flow-components/vaadin-spreadsheet-flow-parent/vaadin-spreadsheet-flow/src/main/java/com/vaadin/flow/component/spreadsheet/Spreadsheet.java

Lines 1181 to 1184 in 7249a90

	private void updateAppId() {
	Optional.ofNullable(UI.getCurrent()).ifPresent(ui -> {
	getElement().setProperty("appId", ui.getInternals().getAppId());
	});

flow-components/vaadin-charts-flow-parent/vaadin-charts-flow/src/main/java/com/vaadin/flow/component/charts/ChartOptions.java

Lines 48 to 57 in 7249a90

	private void updateOptions() {
	UI ui = UI.getCurrent();
	if (ui == null) {
	return;
	}
	JsonObject configurationNode = getJsonFactory()
	.parse(ChartSerialization.toJSON(this));
	ui.getElement().executeJs(

flow-components/vaadin-dialog-flow-parent/vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java

Lines 1052 to 1056 in 7249a90

	private void attachComponentRenderer() {
	this.getElement().executeJs(
	"Vaadin.FlowComponentHost.patchVirtualContainer(this)");
	String appId = UI.getCurrent().getInternals().getAppId();

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:

flow-components/vaadin-dialog-flow-parent/vaadin-dialog-flow/src/main/java/com/vaadin/flow/component/dialog/Dialog.java

Lines 762 to 772 in 7249a90

	private UI getCurrentUI() {
	UI ui = UI.getCurrent();
	if (ui == null) {
	throw new 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.");
	}
	return ui;
	}

Additional context

Originally discovered by @archiecobbs in vaadin/flow#20240 (comment)

Related #6751

[vursen]

Similar issue for LitRenderer specifically: #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.