Fix data race between send_updated_menu and send_initial_data

[basilgello]

The preparation of XDG menu dat still takes place in separate thread,
but since 'send_updated_menu' callback is fired every time XDG menu
provider completes reloading, sending the 'xdg_menu' from
'send_initial_data' is redundant and introduces a data race:

send_initial_data: thread of 'send_xdg_menu_data' is started
menu provider finishes loading XDG data and triggers 'send_updated_menu'
The proper menu is sent from 'send_updated_menu'
send_xdg_menu_data in thread sends empty array to client

Depending on which thread wins, HTML5 client receives either non-empty
or empty array as the last.

[totaam]

You have removed ss.send_setting_change("xdg-menu", xdg_menu) without replacing it with anything. That's not right.
It is never going to send anything if the menu data is already loaded at that point.
menu_provide.get_menu_data() will not fire the menu loading callbacks unless force_reload is set (and it isn't by default):

xpra/xpra/server/menu_provider.py

Line 154 in 075e0e4

if not self.menu_data or force_reload:

send_xdg_menu_data in thread sends empty array to client

I believe this is a bug: 005711f
Doesn't that fix both cases?
The only downside that I can see is that we can send the menu data twice. Meh.

[basilgello]

There is definitely a race condition - you can see that yourself running HTML5 client under Developer tools and annotating the ctx.process_xdg_menu() callers.

It is never going to send anything if the menu data is already loaded at that point.

Why does it work for me properly then, without sending twice?

Doesn't that fix both cases?
The only downside that I can see is that we can send the menu data twice. Meh.

Right, and this means we dont get rid of data race, just send equal result by all racers. Maybe the proper solution is to ensure the self.menu_provider.on_reload takes place every time the load is complete?

received xdg start menu data Utilities.js:62:16
Object { "Довідка": {…}, "Застосунки": {…}, "Ігри": {…}, Applications: {…} }
Client.js:2015:10
received xdg start menu data Utilities.js:62:16
Object { "Довідка": {…}, "Застосунки": {…}, "Ігри": {…}, Applications: {…} }

[totaam]

There is definitely a race condition

I'm not saying there isn't.

Why does it work for me properly then, without sending twice?

I'm saying that it looks like the code would never sends anything if you connect after all the menus have already loaded after you remove ss.send_setting_change.

Right, and this means we dont get rid of data race, just send equal result by all racers.

Exactly.

Maybe the proper solution is to ensure the self.menu_provider.on_reload takes place every time the load is complete?

I think that this is also racy, just in different ways. In fact, I'm pretty sure that this is what I had done and hit some bigger issues.

[basilgello]

I see the only valid case to support two methods is the support of older clients not capable of utilizing send_updated_menu. Or proper solution is cache menu once and send the cached response everytime, and keep sending updated menu only on reloads.

[totaam]

I see the only valid case to support two methods is the support of older clients

Yes, that's why it's there.
Since the xdg-menu-update feature was already present in v3.0.x, I'm OK with dropping the legacy method.

[basilgello]

I'm OK with dropping the legacy method

We can just wrap the sending code for legacy path in something like if not self.xdg_menu_update: check in case you don't want to drop it fully

[totaam]

I believe that this is fixed. Correct me if I am wrong.