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

Blocking rendering on IPC is a bug? #12098

Closed
YurySolovyov opened this issue Mar 1, 2018 · 21 comments
Closed

Blocking rendering on IPC is a bug? #12098

YurySolovyov opened this issue Mar 1, 2018 · 21 comments

Comments

@YurySolovyov
Copy link
Contributor

  • Electron version: All
  • Operating system: All Supported

Following this thread: https://twitter.com/NumaanAshraf/status/968496732278374400:

image

How to reproduce

I know this behaviour is due to some technical constraints, but shouldn't there be a way to not block main thread while handling IPC?

Or at least do the work in chunks.
This could be a major responsiveness booster for a lot of apps out there.

Can someone enumerate why is it the way it is?
Maybe this way we'll have some ideal for possible solutions to make it block less or not block at all?

@sofianguy
Copy link
Contributor

Thanks for reaching out!

Because we treat our issues list as the team's backlog, we close issues that are questions since they don't represent a task needing to be completed. For most questions about Electron there are a lot of options.
Check out the Electron community. There are also a bunch of helpful people in this community forum that should be willing to point you in the right direction.

@YurySolovyov
Copy link
Contributor Author

There are potential action items here, once some questions are answered.

@sofianguy sofianguy reopened this Mar 1, 2018
@ckerr
Copy link
Member

ckerr commented Mar 4, 2018

Related reading material from that twitter thread:

https://medium.com/actualbudget/the-horror-of-blocking-electrons-main-process-351bf11a763c

@YurySolovyov
Copy link
Contributor Author

@YurySolovyov
Copy link
Contributor Author

One possible way to make this better without touching/breaking too much code is to use node's ability to create IPC server via net module (and yes, they are cross-platform).

We can have both IPC implementations supported, but in the docs, we can encourage people to use newer node-based one unless they have very specific reason not to.

If that will help, I can do a prototype with comparison of two IPC approaches

@YurySolovyov
Copy link
Contributor Author

YurySolovyov commented Apr 20, 2018

In addition to net module having cross-platform IPC server/sockets, ws module supports domain sockets as well. This means that it should be possible to create an IPC mechanism using websockets for data transport.

Answering obvious question on why this should be in core and not in userland modules - I think electron should have a decent IPC that is truly non-blocking so it will benefit a lot of apps that receive a critique of being slow and unresponsive lately (sure, IPC is not always to blame, I get that).

I'd be happy to work on the prototype/discuss API unless there are strong objections on such approach.

@YurySolovyov
Copy link
Contributor Author

Important note - we need to be very careful with permissions on such pipes, to make sure that (by default) pipes are readable/writeable only by app itself. Kudos to @paulcbetts for bringing that up

@magne4000
Copy link
Contributor

If anyone wants to easily reproduce the issue, I made test app from electron-quick-start: https://github.com/magne4000/electron-quick-start/pull/3
There a 2 available stress tests for now:

  • One that stresses the main process (and we can see the renderer being blocked)
  • One that stresses a process forked from the main process (no more blocking)

@tommoor
Copy link
Contributor

tommoor commented Nov 2, 2018

I'm really beginning to think this is a serious limitation of Electron's architecture that is relatively unknown and doesn't only show itself when explicitly calling IPC.

For example we were doing currentWindow.getSize() inside of a throttled resize handler and it still completely locked the renderer while resizing a window, took a long time to realize what was going on. (That getSize uses IPC behind the scenes because it's remote)

@jlongster
Copy link

@YurySolovyov what would it look like to restrict permissions of the file to that specific app? How do you do that?

I'm converting to a domain socket with websockets, but I'm not sure what the extra security would bring. If you can access the websocket file, you most likely can access the whole sqlite db that has all my app's data anyway.

@YurySolovyov
Copy link
Contributor Author

@jlongster I think you can set permissions for the pipe to be only readable/writeable by certain user or group just like you would for plain files.

@jlongster
Copy link

You can, but that doesn't restrict it to the app specifically. Unless you're storing encrypted data that is unencrypted at launch, I don't think permissions is really an issue. Anybody who can get to that file can probably just read your application data directly.

@YurySolovyov
Copy link
Contributor Author

I see. Not sure, but you may look at file locking (flock ?) on linux and mac.
Not sure if it works on pipes the same way it works on files.
Also, file locking is tricky stuff and could be hard to get right. 🤷‍♂️

@jlongster
Copy link

I have another question - what can we do on Windows? I've implemented unix domain sockets on macOS but then realized that it won't work on Windows. What is a secure ICP mechanism on Windows?

@YurySolovyov
Copy link
Contributor Author

Node uses named pipes on win. They should work the same for the most part, the only difference is naming scheme/format.

See https://nodejs.org/api/net.html#net_identifying_paths_for_ipc_connections

@jlongster
Copy link

Very nice, thanks! I am currently using the ws library which doesn't support anything like that. Looking forward to moving towards that.

@YurySolovyov
Copy link
Contributor Author

I think ws has the ability to listen on domain sockets.

@nitsakh
Copy link
Contributor

nitsakh commented Mar 8, 2019

This no longer happens after electron 5.0 beta 3 AFAICT. Please reopen otherwise.

@nitsakh nitsakh closed this as completed Mar 8, 2019
@samuelmaddock
Copy link
Member

@nitsakh do you know what changed to fix this?

@ckerr
Copy link
Member

ckerr commented Mar 9, 2019

@nitsakh what process are you using to test? I may be looking at the wrong thing, but I'm think still seeing performance lag in 5.0.0-beta.5 similar to the results when testing in Electrons 3 and 4. I tested this using @magne4000's repro, enabled nodeIntegration and webviewTag to match 5.0's new defaults, then clicked the "start stress (main process)" button, then clicked between the [basic], [entrances], [exits], [text], [attention], [background] tabs. After 5-10 seconds into the stress test, I see a lot of lag when clicking between tabs.

@MarshallOfSound
Copy link
Member

MarshallOfSound commented Mar 9, 2019

@ckerr Completely blocking the main process is going to make the app freeze no matter what we do.

This is the demo I made as an example of the renderer IPC changes in C73

https://gist.github.com/MarshallOfSound/6653351ec40c750e8c2a0336d0c6ed02

Run that gist in Fiddle for 3/4 and then 5.0.0-beta.5, in 3/4 the renderer stops updating whereas 5 keeps the renderer updating even while the main process is blocked. The app still "hangs" after a while but the UI keeps renderering

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

No branches or pull requests

10 participants