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

Use async IPC instead of synchronous #54

Open
lexidevs opened this issue Aug 9, 2024 · 1 comment
Open

Use async IPC instead of synchronous #54

lexidevs opened this issue Aug 9, 2024 · 1 comment
Labels
enhancement New feature or request

Comments

@lexidevs
Copy link

lexidevs commented Aug 9, 2024

Description of feature

Currently, nody-greeter uses ipcRenderer.sendSync in the BrowserWindow process to request (and send) certain data from the main process. While this does simplify development, it has a number of drawbacks:

  • Because JavaScript is single-threaded, sendSync blocks the entire renderer process until a reply is received. For most of the data received, this doesn't pose too much of an issue, because the requests are sent before the windows start. However, other data (e.g. battery, brightness) is requested while the windows are already rendered.
  • This behavior is different from that of web-greeter. Synchronous communication is essentially impossible with Qt5 WebEngine, meaning web-greeter must work asynchronously.

Possible solutions

  • The BrowserWindow could request data from the main process when it first loads using the asynchronous ipcRenderer.invoke method. Then, it could cache the data, and when data does change (say, the brightness_update signal is emitted), asynchronously update the value automatically. This way, getters would still work synchronously, but they would just be getting stored data from cache. Setters could work in the same way: use ipcRenderer.invoke to set a new value in the main process. Other functions (e.g. lightdm.authenticate) could continue as they are, but be supplemented by a new async version that uses invoke instead.
  • The optimal solution would be to replace all functions that currently call sendSync with async versions that return promises. Unfortunately, this would not be compatible with most themes.

This problem has been brought up before in #13, with the response

However, I don't know if I like to use asynchronous I/O operations instead of sync ones... I think this change is unnecessary as almost every I/O operation is done before the windows are started; the exceptions are the battery and brightness features, which didn't seem to block the Renderer process.

However, this isn't exactly correct. Getting any data (battery, brightness, in_authentication, etc) does block the Renderer process. Further, nothing is stopping a theme from getting data at any point, with all the performance consequences thereof. Synchronous communication just isn't a good idea.

@lexidevs lexidevs added the enhancement New feature or request label Aug 9, 2024
@JezerM
Copy link
Owner

JezerM commented Aug 14, 2024

I understand the concerns about synchronous code, and sure it will block the renderer process. However, I prefer it that way now and I don't think it's a big performance issue, when even an old crappy laptop (Intel i3 second gen) can handle this with no problem. Also, async stuff just complicated my whole life while working with web-greeter xd

The first solution seems like the better approach, and that's how Qt5WebEngine does it as well anyways so it might be great for compatibility.

I won't make this change, but if someone makes it and looks good enough to not complicate things in the future, then go for it~ I'd appreciate that.

@JezerM JezerM pinned this issue Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants