Bug when using transport long polling for push updates

[andreasbean]

Uidl updates are expected to be wrapped into

public static final String JSON_COMMUNICATION_PREFIX = "for(;;);["; public static final String JSON_COMMUNICATION_SUFFIX = "]";

Now, from time to time it happens that two or more uidl updates are concatenated into one message when using long polling. While the leading and trailing pre/suffix is removed the suffix and prefix at the concatination point remain leading to the message "An invalid JSON war received..."

Please fix the client so that it can handle more than one message at once or fix the server to prevent more than one messages in a long poll response.

[gilberto-torrezan]

Hi @andreasbean, we had this issue a while ago, but it was caused by an internal bug in Flow that is now fixed. It was first reported in this ticket.

Which version of Flow are you using? Can you share a sample code where the error still occurs?

[andreasbean]

Hi gilberto, I'm using flow 1.0.5. I have push mode MANUAL and transport LONG POLL This is the code which causes the issue

I'm sorry for the formatting, I can't figure out why it doesn't work here at github...

`

private void OnUserListChanged(String id) { getUI().ifPresent(ui -> ui.access(() -> {

                UpdateUsersOnline();

                ui.push();
            })
    );
}

private void UpdateUsersOnline() {

    if (users_online == null) {
        return;
    }

    users_online.removeAll();

    List<String> users = Broadcaster.getUsersInfo();

    for (String user : users)
    {
        Label lbl = new Label(user);

        users_online.add(lbl);
    }
}`

Interestingly it works find the first two updates after starting the server and the third time it fails.

[gilberto-torrezan]

I can't copy-paste the code as is to test, since there are some unknown dependencies there (like Broadcaster and users_online). Are you using Spring or any other framework on top of Vaadin?

I'll try to reproduce it.

[andreasbean]

The code is part of a big project. I can't give you a compiling part out of it, but you can create a list of strings with random content easily. I don't use Spring. Any idea why two messages are received as one? users_online is just a VerticalLayout.

[gilberto-torrezan]

I tried to reproduce the error by using this code, with no success:

@Route("")
@Push(value = PushMode.MANUAL, transport = Transport.LONG_POLLING)
public class MainView extends Div {

    private Div container;

    public MainView() {
        container = new Div();
        NativeButton button = new NativeButton("Add labels via push",
                event -> createThreads());
        add(button, container);
    }

    private void createThreads() {
        for (int i = 0; i < 100; i++) {
            new Thread(() -> addLabel()).start();
        }
    }

    private void addLabel() {
        long id = Thread.currentThread().getId();
        getUI().ifPresent(ui -> ui.access(() -> {
            container.removeAll();
            for (int i = 0; i < 10; i++) {
                container
                        .add(new Label("Label added via push by thread " + id));
            }
            ui.push();
        }));
    }
}

(Using Flow 1.0.5)

Is there anything fundamentally different from your example?

[andreasbean]

Hi Gilberto, thank you very much for working on that. I copied your example into my app and it doesn't crash either.

One difference is that in my app I register the UI to a broadcaster class. In the listener I update the ui and push it to the client.

I call ui->access(). Isn't that sufficient when calling from another thread? It only happens when I open more than one session and post updates to more then one ui.

Thanks for tracking that down with me. Andreas

[gilberto-torrezan]

Feels like something is getting mixed in the process. Maybe you are trying to change the components of a different UI while doing ui.access() (since the error only occurs when running more than one UI) ?

What happens when you use a different Push transport? Do you get a different error or does the app just work?

[andreasbean]

When I switch to websocket transport it works without any issues. I will check again if I have somewhere missed to wrap the UI update in a ui.access(). If it turns out that all updates are in a ui.access() I will try to setup a simple app to reproduce the issue. What's a good starting point for this simple app?

[Artur-]

Run the server using -ea and you will get exceptions if there are missing access() calls or similar locking problems

[mpilone]

I'm seeing the same issue in my application using Flow 1.2.3. I have Push configured for automatic and long-polling.

In a view, I grab the UI in onAttach as it was my understanding that getUI() is not thread-safe (although the test case above uses it in a background thread).

@Override
  protected void onAttach(AttachEvent attachEvent) {
    super.onAttach(attachEvent);

    // Store the UI for access in a background thread.
    ui = getUI().orElse(null);
}

On the client side, I have a "refresh" button that clears a data provider and then kicks off a request to some backend services for data.

private void onRefreshClick() {
    clusterServiceRegistry.clear();
      refreshDataProviders();

  var command = new ClusterServiceCommand(ClusterServiceCommand.Command.QUERY);
    clusterServiceGateway.issueServiceCommand(command);
}

private void refreshDataProviders() {
    recordDataProvider.getItems().clear();
    recordDataProvider.getItems().addAll(clusterServiceRegistry.getRecords());
    recordDataProvider.refreshAll();
}

The backend data is then received in a separate thread and, via UI.access, the data provider is updated.

public void registryChange(ClusterServiceRecord record) {
   ui.access(() -> {
          refreshDataProviders();
      });
}

It appears that if a backend services responds quickly enough, the ui.access can occur before the response to the initial "refresh" button click so both the JSON response to clear the data and the JSON response with the updated data provider are sent in the reply creating invalid JSON.

[{"syncId":11,"clientId":6,"meta":{"async":true},"changes":[{"node":18,"type":"put","key":"size","feat":1,"value":2}],"execute":[[[0,21],"if($0.$connector) $0.$connector.reset();"],[[0,12],"if($0.$connector) $0.$connector.reset();"],[[0,18],2,"$0.$connector.updateSize($1)"],[[0,18],0,1,"$0.$connector.clear($1,$2)"],[[0,18],0,[1,[{"key":"1","col1":"mikes-n-mac.local","col2":"MASTER","col0":"contentDelivery","col3":"DESKTOP","col4":"be87dc797db5cb784fa9772a73a52727"},{"key":"3","col1":"mikes-n-mac.local","col2":"MASTER","col0":"watchdog","col3":"DESKTOP","col4":"fd2d39ac13e811c69c5de2e76fe9695c"}]],"$0.$connector.set($1,$2)"],[[0,18],7,"$0.$connector.confirm($1)"],[[0,21],1,"$0.$connector.updateSize($1)"],[[0,21],0,[1,[]],"$0.$connector.set($1,$2)"],[[0,21],7,"$0.$connector.confirm($1)"],[[0,12],2,"$0.$connector.updateSize($1)"],[[0,12],0,[1,[]],"$0.$connector.set($1,$2)"],[[0,12],7,"$0.$connector.confirm($1)"]],"timings":[4942,6]}]for(;;);[{"syncId":12,"clientId":6,"meta":{"async":true},"changes":[{"node":18,"type":"put","key":"size","feat":1,"value":3}],"execute":[[[0,21],"if($0.$connector) $0.$connector.reset();"],[[0,12],"if($0.$connector) $0.$connector.reset();"],[[0,18],3,"$0.$connector.updateSize($1)"],[[0,18],0,2,"$0.$connector.clear($1,$2)"],[[0,18],0,[1,[{"key":"2","col1":"mikes-n-mac.local","col2":"MASTER","col0":"contentManagement","col3":"DESKTOP","col4":"d02184cbf840c12932fbb076c6954f22"},{"key":"1","col1":"mikes-n-mac.local","col2":"MASTER","col0":"contentDelivery","col3":"DESKTOP","col4":"be87dc797db5cb784fa9772a73a52727"},{"key":"3","col1":"mikes-n-mac.local","col2":"MASTER","col0":"watchdog","col3":"DESKTOP","col4":"fd2d39ac13e811c69c5de2e76fe9695c"}]],"$0.$connector.set($1,$2)"],[[0,18],8,"$0.$connector.confirm($1)"],[[0,21],1,"$0.$connector.updateSize($1)"],[[0,21],0,[1,[]],"$0.$connector.set($1,$2)"],[[0,21],8,"$0.$connector.confirm($1)"],[[0,12],3,"$0.$connector.updateSize($1)"],[[0,12],0,[1,[]],"$0.$connector.set($1,$2)"],[[0,12],8,"$0.$connector.confirm($1)"]],"timings":[4942,6]}]

Like the original bug report, it seems to work for the first request to the application's 'refresh' button, then fails on the second. I don't know if that's just a coincidence with the backend service response timing or related to the bug but the fact that @andreasbean is seeing similar behavior makes me think it is related to the bug.

[mpilone]

Here's a test case that appears to reproduce the problem. The first click on the button usually works while the second click throws the JSON error. It looks like it is related to multiple threads competing for UI.access. I used a Grid and DataProvider because that's what my application is doing but I don't know if it is required to trigger the issue.

@Route(value = "")
@Push(transport = Transport.LONG_POLLING)
public class MainView extends VerticalLayout {

  private UI ui;
  private ExecutorService executor;
  private final ListDataProvider<Item> dataProvider;
  private final List<Item> itemRegistry = new LinkedList<>();

  public MainView() {

    dataProvider = new ListDataProvider<>(new LinkedList<>());
    Grid<Item> grid = new Grid<>(Item.class, false);
    grid.setDataProvider(dataProvider);
    grid.addColumn(Item::getFirstName).setHeader("First Name");
    grid.addColumn(Item::getLastName).setHeader("Last Name");

    Button button = new Button("Click me", event -> {
      synchronized (itemRegistry) {
        itemRegistry.clear();
      }

      dataProvider.getItems().clear();
      dataProvider.refreshAll();

      for (int i = 0; i < 5; ++i) {
        executor.submit(this::doWork);
      }
    });

    add(button);
    add(grid);
  }

  @Override
  protected void onAttach(AttachEvent attachEvent) {
    super.onAttach(attachEvent);

    executor = Executors.newFixedThreadPool(10);

    this.ui = getUI().orElse(null);
  }

  @Override
  protected void onDetach(DetachEvent detachEvent) {
    executor.shutdown();
    super.onDetach(detachEvent);
  }

  private void doWork() {

  try {
      Thread.sleep((int) (Math.random() * 5));

      // Simulate some new piece of data coming in on a background thread and getting put into the display.
      List<Item> copy;
      synchronized (itemRegistry) {
        Item item = new Item();
        item.setFirstName("First " + System.currentTimeMillis());
        item.setLastName("Last ");
        itemRegistry.add(item);

        // Copy the list so we're not accessing the shared registry concurrently.
        copy = new ArrayList<>(itemRegistry);
      }

      ui.access(() -> {
        dataProvider.getItems().clear();
        dataProvider.getItems().addAll(copy);
        dataProvider.refreshAll();
      });

    }
    catch (InterruptedException ex) {
      ex.printStackTrace();
    }

  }

  private static class Item {
    private String firstName;
    private String lastName;

    public String getFirstName() {
      return firstName;
    }

    public void setFirstName(String firstName) {
      this.firstName = firstName;
    }

    public String getLastName() {
      return lastName;
    }

    public void setLastName(String lastName) {
      this.lastName = lastName;
    }
  }
}

[japhill]

Hi, I have been watching this issue for a while because it also affects a project that I am working on. Is there a fix for it yet, or at least a work around until a fix is in? The project that I am working on requires long polling and UI updates from threads.

[KasparScherrer]

I am affected by this as well, but I use the default Transport for push which is WEBSOCKET_XHR.

My workaround at the moment is to wait a bit each time before calling ui.access(..) by letting the Thread sleep. Obviously letting the user wait longer than necessary is not the best solution, but at least it works ;)