More convenient async API by removing UI.access()

[Osiris-Team]

Describe your motivation

Right now doing async changes to the UI requires a lot of boilerplate, for example updating the text of a button asynchronously requires this:

@Push(PushMode.AUTOMATIC)
@Route("")
//...
UI ui = UI.getCurrent();
new Thread(() -> {
  for (int i = 0; i < 5; i++) {
      final int i_final = i;
      ui.access(() -> 
          button.setText("Count: " + i_final)
      );
      Thread.sleep(1000);
  }
}).start();

• Now this gets exponentially worse if we have more things we want to update at different places.
• Its also annoying to create final versions of mutable variables
• In larger codebases and more complex event driven code it can be easy to forget to call ui.access

Describe the solution you'd like

UI ui = UI.getCurrent();
AsyncUI.execLater(ui, () -> {
  for (int i = 0; i < 5; i++) {
      button.setText("Count: " + i)
      Thread.sleep(1000);
  }
});

Implementation could look like this:

• The base Idea is that ui.access is done in another thread in the background in an intervall, hidden from the application developer.
• This also requires caching changes to the UI instead of applying them directly.
• As you can see there are some non-existing methods we are relying on that also would need to be implemented, thus think of this more as pseudo-code.

public class AsyncUI {
    public static Executor executor = Executors.newCachedThreadPool();
    
    public static void execLater(UI ui, Runnable code){
        executor.execute(() -> {
                while(code.isRunning()){
                    ui.access(() -> {
                        for(CachedChange change : ui.getCachedChanges()){
                            change.apply();
                        }
                        ui.push();
                    }
                    Thread.sleep(250);
                }
        });
        executor.execute(() -> {
            try{
                VaadinSession.setCurrent(ui.getSession());
                UI.setCurrent(ui);
                code.run();
            } finally {
                VaadinSession.setCurrent(null);
                UI.setCurrent(null);
            }
        });
    }
}

v1

Describe the solution you'd like

Instead we could assign the new Thread to the UI:

UI ui = UI.getCurrent();
new Thread(() -> {
  ui.linkThread(Thread.currentThread());
  for (int i = 0; i < 5; i++) {
      button.setText("Count: " + i)
      Thread.sleep(1000);
  }
  ui.unlinkThread(Thread.currentThread());
}).start();

• linkThread() would potentially wait if there was is an existing linked thread, so that synchronized orderly access is still guaranteed. This isn't optimal in my opinion, I assume there is a better way.
• unlinkThread() would be optional if we expect the thread to stop soon, since internally unlink will be called when the thread dies.

[Legioth]

The benefit is quite small since the described code example has just one line less (from not needing to explicitly re-declare a final variable). There's still one line to "lock" and one line to "unlock". At the same time, there's a big drawback since this format has a high risk of bugs if the developer forgets to call unlinkThread.

I don't think it makes sense to assume that the thread will stop soon due to the way background worker threads are often managed in a thread pool rather than started on demand (until virtual threads get wider adoption). Another challenge is that I don't think there's any reliable way for the framework to know when a thread has been stopped other than repeatedly running thread.isAlive(). There's also a more subtle related bug in that code example which is that it would make the UI block any user input for five seconds by doing sleep while keeping the UI locked.

I think there's a better solution to on the horizon: use signals for UI state management. The way signals are envisioned to work would let them manage mutual exclusion transparently so that the example case could be implemented like this:

NumberSignal count = new NumberSignal<>();
button.setText(() -> "Count: " + count));

new Thread(() -> {
  for (int i = 0; i < 5; i++) {
      count.incrementBy(1);
      Thread.sleep(1000);
  }
}).start();

[Legioth]

There's also an alternative API that exists already today that allows doing the example case with the same amount of code as what I showed for the not-yet-existing signals alternative:

UI ui = UI.getCurrent();
SerializableConsumer<String> setText = ui.accessLater(button::setText, null);

new Thread(() -> {
  for (int i = 0; i < 5; i++) {
      setText.accept("Count: " + i);
      Thread.sleep(1000);
  }
}).start();

This feature is documented in https://vaadin.com/docs/latest/building-apps/presentation-layer/server-push/updates#access-later.

[Osiris-Team]

Didn't know about accessLater that looks pretty neat, thanks for the info!

The drawbacks you mentioned to my approach make sense. What if Vaadin had a modifiable ThreadPool internally and provided direct methods to create async tasks that have also access to the UI, this should fix the issue of needing to check if a thread is alive since instead we simply have a Runnable we execute via the internal ThreadPool. I am guessing we would still have the problem of locking the UI for the complete duration of the async task which defeats the purpose.

Instead what if there was an option to enable an implicit ui access mode so that ui.access() is called, if necessary, inside button.setText() for example?

[Legioth]

I am guessing we would still have the problem of locking the UI for the complete duration of the async task which defeats the purpose.

Exactly. If it's fine keep the UI blocked during the whole duration of the task, then you could in many cases just as well run the task directly from the event listener.

Instead what if there was an option to enable an implicit ui access mode so that ui.access() is called, if necessary, inside button.setText() for example?

This might seem tempting but it turns out to cause more trouble than what it solves. There are two different variations that I'm aware of:

1. Some specifically chosen methods have implicit UI access while other methods still require explicit synchronization. This forces the application developer to keep track of which methods can be used in which way and if enough methods are covered then they might forget to consider that not all methods are covered.
2. If all methods are supposed to be covered, then all methods in all components must contain some boilerplate code to handle synchronization. This includes all add-ons and all custom components in applications. I assume there are many more such methods than there are places that now require explicit ui.access.

Regardless of the variation, there are also cases where you need synchronization between different methods e.g. if you want to set a button as disabled based on whether a text field is empty. This means that you might still in many cases need explicit ui.access which is again more likely to be forgotten if it's not always needed.

[Osiris-Team]

Mhmmm I hoped that you can simply add the internal/implicit ui.access() call inside the Component class that everyone extends from, but I guess its not that simple?

What if ui.access would not lock the UI and instead that responsibility is given the developer? If we look at raw HTML/JavaScript for example you can modify the DOM from any async function at any time without needing to lock it, if I'm not wrong.

Or instead of modifying ui.access, a more non-destructive way would be adding a function similar to ui.linkThread(Thread.currentThread()); that is called ui.promoteToUIThread(Thread.currentThread()); for example.

Another argument for this approach is that I think there are more places we do not need synchronization/locks since in many cases its 1 component <=> 1 async task that modifies it. And if there are multiple async tasks the developer almost always already has to add some sort of synchronization by themselves anyways.

[Legioth]

JavaScript is different since it's inherently single-threaded. await can give the illusion of concurrent change to shared state but you still only need to code in a defensive way in places that use await. With multiple threads in Java, every single line of code that directly or indirectly touches an instance field will have to take concurrency into account unless a more coarse synchronization convention is used.

The core question is how granular the critical sections are. If they would be more granular, e.g. on the method or component instance level, then concurrency becomes everyone's problem all the time in one way or another. If they are less granular, then it's a problem only in the isolated cases that are inherently concurrent because the framework can handle synchronization for the other cases. Maybe those inherently concurrent cases could be further simplified but that should not be at the expense of the non-concurrent cases.

One big benefit with the callback based approach with access is that it helps avoid deadlocks. You can do explicit locking in a way that is compatible with access by using ui.getSession().lock() but then there's a risk that one thread has UI A locked and tries to lock UI B while another thread has B locked and tries to get the lock for A. You would have the same risk with linkThread because it also must wait until the the lock is released (temporarily unlocking one while locking the other will instead lead to other subtle surprises). This doesn't happen with access callbacks because it doesn't block if the lock is held by some other thread but instead leaves the callback in a queue to be run once the current task is finished.

[Osiris-Team]

I see. What about the idea I had about having multiple UI threads and being able to promote new threads to UI threads?

I guess the core of this idea is removing the locking behavior of Vaadin internals, or is this achievable without major changes to the internals?

[Legioth]

I don't see what the difference would be between a "UI thread" and a thread that holds a UI lock - those are just different ways of expressing the concept of mutual exclusion.

That mutual exclusion is necessary so that UI logic can assume that instance field values won't change unexpectedly while the logic is running but only between invocations of that logic.

[Osiris-Team]

I see, so adding an optional feature that promotes a thread and binds it to an UI (so that UI.getCurrent() for example is not null) without locking it is not realistic in Vaadin?

This would bypass the need for the ui.access boilerplate in a new thread and trade in safety for cleaner code and manual control over locking if the developer needs it.

[Legioth]

The framework doesn't need threads to be "promoted" in any way. If you use something that depends on UI.getCurrent and don't want to use ui.access, then you can run UI.setCurrent on your own (and similarly for e.g. VaadinSession.setCurrent and VaadinService.setCurrent). Just remember to clean up after yourself to avoid leaking memory or accidentally sharing UI instances between users.

Not using ui.access means that the smallest mistake in application code can cause subtle thread safety issues, memory leaks and security issues that might all be notoriously tricky to debug and are likely to only show up in production. I don't trust myself enough to develop applications in that way and I'm happy to accept a little bit of boilerplate from ui.access to get rid of those risks. If someone wants to take those risks, then they can create their own helper method for doing it. I don't see why the framework should give any encouragement by providing a ready-made helper.

[Osiris-Team]

Okay got it, so something like this should work without issues:

public class AsyncUI {
    public static Executor executor = Executors.newCachedThreadPool();
    
    public static void execLater(UI ui, Runnable code){
        executor.execute(() -> {
            try{
                VaadinSession.setCurrent(ui.getSession());
                UI.setCurrent(ui);
                code.run();
            } finally {
                VaadinSession.setCurrent(null);
                UI.setCurrent(null);
            }
        });
    }
}

[Legioth]

Should work yes. But I'm curious to understand what you'd do on top of that for the locking and unlocking?

[Osiris-Team]

I hoped that if, for example, a user button click event happens in another UI and at the same time the async task is running, that it won't change the returned values by getSession and getCurrentUI. I doubt this though since it seems that those references (UI and Session) are stored in a map that has their types as keys, for this to work the map keys should additionally include Thread information.

[Legioth]

That map is stored in a TheadLocal which means that it will not be affected by what happens in other threads.

[Osiris-Team]

I was just testing the AsyncUI class and it seems to work without causing exceptions, however in longer running tasks the last updates to the UI are not delivered it seems, any ideas? Even though I explicitly set the push configuration to automatic before.

Regarding locking: in my test case and in most my projects, I only have 1 component that gets updated by 1 async task, so I don't think there is need for locking if I understood correctly.

[Legioth]

The framework doesn't know when to push out changes unless you either explicitly run ui.push() or you unlock the session (through session.unlock() or by returning from a ui.access callback). It's done in this way since it would in most cases be inefficient if each individual change to every component would be pushed out immediately when the change happens.

It is never safe to update components without holding the lock even if no other application logic touches that component at the same time. There's a UI-scoped data structure that keeps track of all components that have changed since the last time changes were pushed out. There will be problems if multiple threads update that structure at the same time or if one thread is collecting changes from those components at the same time that some other thread is making changes to the same component instance. These things can happen at any time through regular user interaction which means that you always need to acquire the lock.

[Osiris-Team]

Okay so if I could cache the changes instead of applying them directly and have another thread that runs in an Intervall that locks and pushes those cached changes then unlocks, the AsyncUI class should be safe to use.

[Legioth]

Yeah. Though I thought you were trying to find a way of making things more easy and convenient, not to add another 5 layers of complexity?

At the end of the day, you still need some way of indicating which lines of code are UI changes and when to apply those changes. How do you plan to do that in a way that is more convenient than through ui.access?

[Osiris-Team]

Yeah haha I hoped there would be an easy way with some sort of trick to achieve this. But if you say it's not possible without adding some sort of additional wrapper around all components, it makes no sense, since as you said my aim is to get rid of explicitly calling UI.acess all the time and make the code cleaner.

Would it be possible to add this caching to the framework directly maybe?

But at least now I have a more realistic and achievable feature request, will update the main comment in a bit.

[Legioth]

It doesn't matter if the code is in the framework or helper methods/classes in application code or a library. Application code still needs to somehow designate which operations are UI changes and when changes should be applied and pushed out to the client.

Something.cacheUiChangeForLater(() -> changeUI() is exactly the same structure as ui.access(() -> changeUI()) so it doesn't really change anything. Alternatively, you would have to design a completely new API defining changes so that button.setText("foo") would become Something.cacheUiChangeForLater(button, SET_TEXT, "foo") which doesn't really make sense.

[Osiris-Team]

I just updated the main/top comment, hope that makes it clearer. The app developer could additionally define the interval between updates/pushes.

[Osiris-Team]

Alternatively, you would have to design a completely new API defining changes so that button.setText("foo") would become Something.cacheUiChangeForLater(button, SET_TEXT, "foo") which doesn't really make sense.

Yeah I think this might be what's needed if you say so. However this change can be done internally and would only be needed in tasks running from AsyncUI for example, avoiding a breaking API change.

[Legioth]

But is it really worth it to design two APIs for every single component feature just to be able to avoid a little bit of boilerplate in wrapping some code snippets in ui.access(() -> { ... });? That sounds like quite serious overkill.

[Osiris-Team]

Yeah maybe instead of caching, it could be easier to make the internal data structures affected by this thread safe in general or only thread safe in the case when the task is from AsyncUI.

[Legioth]

I don't see how that could be done without making it so that all components implementations, including all views in all applications, need to take thread safety into account all the time. The whole point of coarse-grained locking on the "transaction" level is to make it so that individual operations don't need to take thread safety into account.

[Osiris-Team]

Yeah I don't see it either, how this could be made simpler to implement, feel free to close as not planned.