Vaadin way: mention background threads

[mvysny]

Doing threading right is vital but very hard and surprisingly lot of customers do it the wrong way. I think Vaadin way should definitely tackle this topic. A very basic proposal is attached.

In general, Vaadin UI framework (and many other UI frameworks, including Swing) is not thread-safe. It's not safe to call any UI code other than ui.access() from background threads. Doing it will introduce subtle threading bugs. Those are the nastiest types of bugs since they occur randomly and usually in production under heavy load, and therefore are almost impossible to reproduce and fix in a controlled dev environment.

That implies that:

1. Background threads must not call any methods on UI (including static stuff like UI.getCurrent()).
2. By extension, if you're using MVP, background threads must not call any methods on presenter

The best way to run background threads from the UI is to follow these rules:

1. When a background thread is started from an UI action, the UI would gather all necessary data (such as current user) into an immutable object, which is then passed around in the thread - either as a parameter, or into a ThreadLocal.
2. The background thread can receive a reference to the current UI as a parameter, but can only use it to call ui.access(). Even better is to wrap the current UI in an Executor so that the UI instance is not exposed and programmers are not tempted to call additional methods on the UI instance. The thread would then receive the Executor instance.

Additional links:

1. https://vaadin.com/docs/v23/advanced/long-running-tasks#handling-long-running-tasks-asynchronously

CC @peholmst

[peholmst]

@mvysny Thanks for your input! I was actually about to start writing about background threads today. Will take your comments into account.

[TatuLund]

Good discussion opening, although I am not agreeing the best practice. I would avoid setting thread locals in the background threads at all cost and not passing UI instance there.

I prefer to write in the views method that uses ui.access using getUI of the component to get the ui instance, hence I do not need UI.getCurrent()

https://github.com/TatuLund/vaadin-create23/blob/master/vaadincreate-ui/src/main/java/org/vaadin/tatu/vaadincreate/crud/BooksView.java#L226

In Flow getUI returns optional, which is even better

getUI().ifPresent(ui -> ui.access(() -> { ... }));

That makes it clean to use background threads in Presenter

https://github.com/TatuLund/vaadin-create23/blob/master/vaadincreate-ui/src/main/java/org/vaadin/tatu/vaadincreate/crud/BooksPresenter.java#L76

I find this one of the few things in typical Vaadin application where separation of concern serves purpose. With this arrangement Presenter does not need know about UI, the background thread can be business logic only and the view does not need to care about business logic.

[mvysny]

Good points!

Unfortunately getUI() is not thread-safe and must not be called from the background thread. The reason is that it may trigger Composite lazy-initialization, which is not something you want to be running in a non-UI thread. The only thread-safe method on UI is ui.access(), and perhaps ui.getSession(). It is unfortunate that Vaadin's javadoc doesn't mention thread-safety.

The ifPresent() is IMHO an anti-pattern: if the UI isn't available then it silently does nothing, which is a source of random bugs: either the UI should always be there (and then the update should succeed) or it's not there.

I agree ThreadLocal is perhaps not a good practice; in such case the thread should receive an UI Executor by other means. I guess that you'll have a background thread Executor executing jobs; the job should then receive UIExecutor as a constructor parameter.

The implementation of the UI Executor is very straightforward:

public final class UIExecutor implements Executor {
    @NotNull
    private final UI ui;

    public UIExecutor(@NotNull UI ui) {
        this.ui = Objects.requireNonNull(ui);
    }

    @Override
    public void execute(@NotNull Runnable command) {
        ui.access(command::run);
        // maybe handle UIDetachedExceptions in some clean way
    }
}

[TatuLund]

Unfortunately getUI() is not thread-safe and must not be called from the background thread.

It is thread safe. That was exactly my point.

[TatuLund]

The ifPresent() is IMHO an anti-pattern: if the UI isn't available then it silently does nothing, which is a source of random bugs: either the UI should always be there (and then the update should succeed) or it's not there.

You can also use

getUI().ifPresentOrElse(ui -> ui.access(() -> {...}), () -> logger.info("Component detached, e.g. browser closed, updates not pushed") );

The UI or actually the component is not there if user was bored to wait and closed the browser, navigated elsewhere.

[mvysny]

It is thread safe. That was exactly my point.

According to my knowledge it's not. Consider calling Composite.getUI() - it goes to Component.getUI() which calls getParent() which in turn calls getElement(). getElement() is overridden in Composite to return the element of getContent(). getContent() checks local variables content and contentIsInitializing and possibly calls initContent() which then starts populating Composite contents in background thread.

The Composite.getContent() isn't synchronized, the content nor contentIsInitializing aren't even volatile, so changes done by one thread may not be observed by others, leading to possible double-initialization and possible unsafe publication of Vaadin components between multiple threads. In short, this is a race condition. At best, Vaadin component initialization in background threads is a grey area; at worst it should be avoided.

If you believe the above observations are incorrect, I'd be happy to go through a proof. The proof should also show that all calls made by getUI() such as getStateProvider().getComponent(getNode()) are also thread-safe. I highly doubt it since first observation shows that StateNode hierarchy is mutable and held in HashMap which is not thread-safe.

[TatuLund]

Based on my 10 years experience getUI() has worked both in Vaadin 8 and newer versions in a robust way. I have used the pattern I described and endorsed many of our customers to follow it with success.

At best, Vaadin component initialization in background threads is a grey area; at worst it should be avoided.

I somewhat agree. It works most of the time, and I have contributed some bugfix in components to enable this but I would still not recommend.

The principle of separation of concern in the pattern I am endorsing also avoids this problem as you will keep the background threads in your business logic clean from UI code.

[mvysny]

Based on my 10 years experience getUI() has worked both in Vaadin 8 and newer versions in a robust way. I have used the pattern I described and endorsed many of our customers to follow it with success.

Happy to hear that! Unfortunately that doesn't change the fact that getUI() is not thread-safe according to my best knowledge of the Java Memory Model. It might have worked on x86 because x86 memory barriers are flushed eagerly, but it may not work on arm64 or risc where memory barriers are lazily flushed. Also the customers might have encountered random infrequent faults, but it was unreproducible and not really that critical to warrant further investigation.

[TatuLund]

is overridden in Composite to return the element of getContent(). getContent() checks local variables content and contentIsInitializing and possibly calls initContent() which then starts populating Composite contents in background thread.

This is not a problem as my recommendation is not to start background thread in the constructor but in enter() in Vaadin 8 or afterNavigation() in newer versions. I.e. after navigation is complete you are past the security barriers etc. So you are clear to do some backend activity. In constructor or beforeEnter it is premature. At this stage components are attached and getUI() returns correct value, unless the component is detached again (which was stated earlier).

[mvysny]

In this simple case the getUI() will work:

@Route(value = "")
public class MainView extends Composite<Component> implements AfterNavigationObserver {
    @Override
    protected Component initContent() {
        final VerticalLayout vl = new VerticalLayout();
        vl.add(new H4("Hi!"));
        return vl;
    }

    @Override
    public void afterNavigation(AfterNavigationEvent event) {
        System.out.println("HI!");
        MyExecutors.get().submit(() -> getUI().access());
    }
}

The reason for that is that:

1. initContent() is called before afterNavigation()
2. quoting from Executor, Memory consistency effects: Actions in a thread prior to submitting a Runnable object to an Executor happen-before its execution begins, perhaps in another thread.
3. That means that the block running in background thread will see a fully initialized view, because there is a happens-before relation between initContent() and getUI(). One could argue that it's barely visible (and one would be right), but at least the code is correct, with respect to thread-safety.

But the above no longer holds if the view is mutated in any way after the background block is submitted to the Executor, for example removed from the parent. In such case the Component.getParent() called from a background thread will start reading from structures that are not meant to be thread-safe (NodeState and such), and you can receive all sorts of weird behavior. @Legioth could you perhaps spare a thought on this?

[TatuLund]

If you follow my example application further, one additional recommendation I have is to run background tasks as Future and when component is detached I will have something like

public void onDetach(DetachEvent event) {
    presenter.cancelTasks() {
}

E.g. https://github.com/TatuLund/vaadin-create23/blob/master/vaadincreate-ui/src/main/java/org/vaadin/tatu/vaadincreate/crud/BooksView.java#L438

and

https://github.com/TatuLund/vaadin-create23/blob/master/vaadincreate-ui/src/main/java/org/vaadin/tatu/vaadincreate/crud/BooksPresenter.java#L89

The primary concern is not that the view may be composite, but if the component is detached, there is no longer point to push updates and consume thread resources, so we stop them immediately.

This naturally means that if getUI().ifPresentOrElse(..) was used, the else callback is never called unless there is a bug in the framework or something super odd happening. I have not seen that in my projects happening.

[mvysny]

Since getUI() is not protected by a lock, it can run concurrently to your onDetach() function. That means that the component may be detached in one thread, and at the same time its parent examined in another via getUI().

[TatuLund]

Since getUI() is not protected by a lock, it can run concurrently to your onDetach() function. That means that the component may be detached in one thread, and at the same time its parent examined in another via getUI().

It gives only the ui reference and the actual things happen in access block, which has the lock. So nothing that requires lock is executed without one.

[mvysny]

It gives only the ui reference

It examines internal structures such as StateNode without having the UI lock obtained. That is thread-unsafe. The subsequent call of access() block is not significant.

[Legioth]

getUI() is not designed to be thread safe even though the current implementation is typically behaving well enough in most cases. There can be lots of nasty things in theory, e.g. an infinite loop if the component hierarchy is concurrently turned upside down and one of the parent pointers end up with the old value in a cache line whereas the other ones are refreshed.

In addition to the potential races, I also recommend against using it from a background thread because it's a Pandora's box. It's much easier to explain and remember that all Component methods should be avoided from background threads than to have a list of special cases that might be safe under various conditions.

The fix is just to call the method while the session is locked and then referencing that effectively final value from the background thread.

[TatuLund]

So doing it like this instead:

UI ui;

public void onAttach(AttachEvent event) {
   this.ui = event.getUI();
}

public void updateViewAsync(Bean data) {
    if (ui == null) {
        throw new IllegalStateException("Do not call me before attach");
    }
    try {
        ui.access(() -> {
            ... do things here ...
        });
    } catch (UIDetachedException exp) {
        logger.info("UI detached");
    }
}

[Legioth]

That pattern can have a race if the background job is stated e.g. in the constructor and it happens to run before the component has been attached. That's also a potential problem if the background job uses getUI(). There's also a theoretical issue of concurrently reading and writing an instance field that's neither final nor volatile.

If you start the job from the constructor, then you need to use UI.getCurrent() and can then pass that value directly to the job rather than relying on an instance field:

UI ui = UI.getCurrent();
scheduleAsyncUpdate(ui);

It's typically better to start the job from some event (e.g. attach, click or navigation) rather than from the constructor. Specifically when using onAttach, you can also get the UI straight from the event.

@Override
public void onAttach(AttachEvent attach) {
  scheduleAsyncUpdate(attach.getUI());
}

You can use addAttachListener from the constructor instead of overriding onAttach if you want to have a one-liner.

If you do things really properly, then you should also take care of unscheduling if the component is detached. This is particularly important when you set up a continuous subscription rather than a one-time task.

@Override
public void onAttach(AttachEvent attach) {
  Disposable job =  scheduleAsyncUpdate(attach.getUI());
  addDetachListener(detach -> {
    job.dispose();
    detach.unregisterListener();
  });
}

Can also use a separate onDetach but then you again have to use an instance field for passing around the unsubscribe handle.

And don't get me started on he possibility that the component is attached and then detached again during the same round trip...

[TatuLund]

If you start the job from the constructor

If we are here in the quest to find the Vaadin way and best practice, we should anyway recommend not to start the job in the constructor. Like I already pointed that afterNavigation probably is the best suited place. And as you point onAttach perhaps the second best.

And don't get me started on he possibility that the component is attached and then detached again during the same round trip...

Yeah, that is why I prefer afterNavigation.

[TatuLund]

A side topic (we could extract this to enhancement ticket in Flow) we should add Component#access(..) method which reduces the boilerplate needed.

So adding something like this to Component:

UI ui;

protected void onAttach(AttachEvent attachEvent) {
   this.ui = attachEvent.getUI();
}

protected void onDetach(DetachEvent detachEvent) {
   this.ui = null;
}

public void access(Command command) {
    if (ui == null) {
        throw new IllegalStateException("Do not call me before attach");
    }
    try {
        ui.access(command);
    } catch (UIDetachedException exp) {
        logger.info("UI detached");
    }
}