Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ui.accessLater() is confusingly named #20362

Open
mvysny opened this issue Oct 29, 2024 · 1 comment
Open

ui.accessLater() is confusingly named #20362

mvysny opened this issue Oct 29, 2024 · 1 comment

Comments

@mvysny
Copy link
Member

mvysny commented Oct 29, 2024

Describe your motivation

The name of the UI.accessLater() function is highly confusing. Naming-wise it's very close to the access() function, but the semantics are vastly different:

  • access() immediately submits given Command to run on the event queue.
  • accessLater() doesn't submit anything, but instead returns a SerializableRunnable which, when ran by a thread, will run a block via UI.access().

What's confusing is that the function starts with a verb (access), hinting that the function immediately accesses the UI in a thread-safe manner. That can lead to two kinds of programming errors.

First issue: it appears as if UI.getCurrent() is called from a background thread. Take a look at the following example:

button.addClickListener(clickEvent -> {
    Thread.ofVirtual().start(UI.getCurrent().accessLater(() -> {
        // Perform the UI operation here.
    }, null));
});

At first sight, without realizing the subtle difference between UI.access() and UI.accessLater(), I see that a thread is calling UI.getCurrent() which should return null, and therefore the thread should fail with a NPE. However, something else happens: the thread actually runs the block with the session lock held.

The examples at https://vaadin.com/docs/latest/building-apps/presentation-layer/server-push/threads are affected by this issue.

Second issue: this innocent code actually does nothing:

ui.accessLater(() -> { doSomething(); });

Describe the solution you'd like

Rename (introduce new function, deprecate old one) the accessLater() to something like:

  • wrapSafe()
  • toUISafeRunnable()
  • asSafeRunnable()
@mvysny mvysny changed the title Deprecate ui.accessLater() Rename ui.accessLater() Oct 29, 2024
@mvysny mvysny changed the title Rename ui.accessLater() ui.accessLater() is confusingly named Oct 29, 2024
@mshabarov
Copy link
Contributor

vaadin/framework#9718 is related.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants