Data races: Writes in VaadinSession#refreshTransients and reads of those variables are not ordered by synchronization

[sherter]

• Vaadin Framework version: 8.13.0
• Description of the bug: Variables session, service and lock in VaadinSession are not volatile and hence calling the getters from different threads without external synchronization can result in a data race (even though a comment in getSession() explicitly states that there is no need for having a lock when doing this.)

If Vaadin is not going to prevent these data races internally, whats the recommended way to use VaadinSession instances from different threads? We would have to lock on the same monitor as the writes do when calling the getters, but thats kind of a chicken and egg problem then. Or is it expected to see random values (even null, when not published safely) when calling these getters?

These races can also be detected with ThreadSanitizer, e.g.:

WARNING: ThreadSanitizer: data race (pid=796)
  Write of size 4 at 0x0006309cde40 by thread T95:
    #0 com.vaadin.server.VaadinSession.refreshTransients(Lcom/vaadin/server/WrappedSession;Lcom/vaadin/server/VaadinService;)V VaadinSession.java:1531 
    # ?
    #2 com.vaadin.server.VaadinService.loadSession(Lcom/vaadin/server/WrappedSession;)Lcom/vaadin/server/VaadinSession; VaadinService.java:2216 
    #3 com.vaadin.server.VaadinService.getExistingSession(Lcom/vaadin/server/VaadinRequest;Z)Lcom/vaadin/server/VaadinSession; VaadinService.java:928 
    #4 com.vaadin.server.VaadinService.doFindOrCreateVaadinSession(Lcom/vaadin/server/VaadinRequest;Z)Lcom/vaadin/server/VaadinSession; VaadinService.java:773 
    #5 com.vaadin.server.VaadinService.findOrCreateVaadinSession(Lcom/vaadin/server/VaadinRequest;)Lcom/vaadin/server/VaadinSession; VaadinService.java:747 
    #6 com.vaadin.server.VaadinService.findVaadinSession(Lcom/vaadin/server/VaadinRequest;)Lcom/vaadin/server/VaadinSession; VaadinService.java:605 
    #7 com.vaadin.server.VaadinService.handleRequest(Lcom/vaadin/server/VaadinRequest;Lcom/vaadin/server/VaadinResponse;)V VaadinService.java:1602
    ...
    #17 com.vaadin.server.VaadinServlet.service(Ljavax/servlet/http/HttpServletRequest;Ljavax/servlet/http/HttpServletResponse;)V VaadinServlet.java:448

  Previous read of size 4 at 0x0006309cde40 by thread T80:
    #0 com.vaadin.server.VaadinSession.getSession()Lcom/vaadin/server/WrappedSession; VaadinSession.java:398 
    ...
    #5 com.vaadin.server.VaadinService.handleRequest(Lcom/vaadin/server/VaadinRequest;Lcom/vaadin/server/VaadinResponse;)V VaadinService.java:1608 
    ...
    #15 com.vaadin.server.VaadinServlet.service(Ljavax/servlet/http/HttpServletRequest;Ljavax/servlet/http/HttpServletResponse;)V VaadinServlet.java:448

[TatuLund]

I would appreciate if you could describe in more detail your actual use case and what kind of problem this produces to you.

However I noticed that your call stack goes thru findOrCreateVaadinSession, and in that method there was indeed a problem with locking, that could in some corner cases cause dead lock. Probably this is related. The problem has been fixed in Vaadin 8.14.0

https://github.com/vaadin/framework/pull/12355

[sherter]

This doesn't actually cause any problems for me (yet). I'm just trying to understand what "refreshing" means and if it could cause a problem that a thread might never see the "refreshed" values. Data races scare me, when I can't say for sure that they are not a problem 😨

[TatuLund]

If your concern is mainly concurrency with multithreaded environment, then it should be enough for you to know, that when you want to update Vaadin view or something that is "owned" by the http request thread (usual use case is that you have async data loading and updating UI when backend call has been completed), you should use ui.access(..) method to do that. ui.access(..) will wait for getting lock the session, locking the session, runs the update and frees the lock.

Check Vaadin Push training video: https://vaadin.com/learn/training/vaadin-push

And documentation: https://vaadin.com/docs/v8/framework/advanced/advanced-push

[sherter]

This doesn't really answer my question though. UI/VaadinSession.access() makes the same racy read when calling getService(). If this doesn't concern you, I guess we just have to make sure ourselves that we publish the VaadinSession to other threads safely (i.e. ensure that the fields are non-null and establish happens-before) and then just hope that it's okay to work with potentially non-refreshed/stale values.

Thanks anyway for your help!

[TatuLund]

I guess we just have to make sure ourselves that we publish the VaadinSession to other threads

I would like to understand better what you mean by this and why you need to do it?

[sherter]

Maybe this example makes it a bit more clear: MyUI.java.txt

Or are you asking about the meaning of "safe publication" or "data race" in general?

[TatuLund]

Maybe this example makes it a bit more clear:

No, the code looks to me too fabricated and something that is not happening in real apps. So it does not reveal your actual use case and intention.

Why you need to publish the session to other threads? I have a feeling that you are trying to apply some design pattern that is very uncommon amongst Vaadin apps, but you are not able to clearly describe that. Thus probably you looking to this whole thing from a wrong angle.

Let me take a concrete example where you need cross-thread communication in application. Say I have separate REST end point the application, and certain calls need interaction with UI. I happen to have simplified example of this here

https://github.com/TatuLund/cdi-demo/tree/master/src/main/java/org/vaadin/cdidemo

The demo app has REST end point, where you can do GET with hello. The end point posts that as event to a event bus, which in turn is listened by main view of the application, and in case event is fired shows the hello message to the user. This app demonstrates that with sufficient separation of concern, all parties involved with this interaction are relatively agnostic to each other and e.g. no publishing of VaadinSession anywhere is needed.

(Your actual use case can be totally different, but I would assume that similar principles can and should be applied)

Side note, you do not need to override VaadinServletService in order to set session init listener, you can do it in servletInitializded directly, see

https://github.com/TatuLund/cdi-demo/blob/master/src/main/java/org/vaadin/cdidemo/MyVaadinUI.java#L177

[sherter]

Use case: Iterate over all UIs of a VaadinSession and show some popup in every UI, triggered by an external event (i.e. outside of any Vaadin request handling logic).

Are you saying that VaadinSession is an internal API that only the framework is allowed to use? I'm not allowed to call VaadinSession#access myself?

[TatuLund]

Use case: Iterate over all UIs of a VaadinSession and show some popup in every UI, triggered by an external event (i.e. outside of any Vaadin request handling logic).

Ok, that is actually fairly similar to what I have in the demo app, where REST module is triggering external event, which is subscribed by the view. I would recommend to use this pattern also for you.

I'm not allowed to call VaadinSession#access myself?

You can. Although there is seldomly need, as more often UI#access is used (like also in my demo app)

I wrap showing of the message in UI#access here, as the showMessage is called in event triggered in other thread (by EventBus)

https://github.com/TatuLund/cdi-demo/blob/master/src/main/java/org/vaadin/cdidemo/views/main/MainViewImpl.java#L115

[sherter]

I had a look at your example. If you would do the eventBus.post(event) call asynchronously from a completely independent thread (i.e. remove the @RequestScoped annotation from DemoEndpoint), you would suffer from the same data race. Instead of the VaadinSession, you capture the UI in your eventListeners, but ui.access() just delegates to VaadinSession.access, which then does the racy read of the service field.

[TatuLund]

Then I would say you should not have real concern, as this kind of implementations are well battle proven.