Bridging layer research for Mapeo-Core-Next

[ErikSin]

Research different architectures for bridging layers for Mapeo-Core-Next. Document the pros and con's of each, and that will be used to inform how we integrate Mapeo-Core-Next. We will eventually prototype the different types of architecture (For each architecture protyped please create a new ticket).

To Do:

• [x] Document (in this ticket) High level architecture of current bridging layer. Try to include the reasons when architectural decisions were made
• [x] Research the best/most common practices for bridging layer and document (in this ticket) reason we may not be able to follow best practices
• [x] Determine what criteria we are using for choosing architecture
• [ ] Create repo using Electron with Mapeo-Core-Next
• [ ] Create tickets detailing what types of architectures we are prototyping

[achou11]

met today to discuss high level architecture. points of discussion included:

• current architecture and the context for why it is:

  

• how Electron's recommendation against node integration affects this architecture, if we decide to keep it. basically, it really should only apply to the web app renderer window and any node-dependent code there should be moved to the main or a "background process". somewhat out of scope for this specific issue though

• relevant recommendations from Electron related to inter-process communication. we already implement this to some extent but it's hard to follow and should be better documented. https://www.electronjs.org/docs/latest/tutorial/message-ports#messageports-in-the-main-process

• prototyping goals:

  • explore alternative architectures vs keeping current architecture but improving it
  • how does rpc-reflector get incorporated in this context?

[tomasciccola]

The main reasoning for running multiple background processes (which are basically hidden windows) is that ipc between renderProcess and mainProcess is blocking, so if a request to a long running task is made from the UI to the mainProcess, it will block until it gets a response. Today I made two tests:

1. I thought maybe the issue was with using ipc.send which was replaced by ipc.invoke which supposedly is non-blocking, but after some tests, it doesn't seem to be the case. At first I thought it was working (by quickly updating the ui with a setInterval while the mainProcess delayed the return with a blocking sleep) but when I also tried with updating the ui with user interaction (with an eventListener on a button) it was clear that the button wasn't updating until the response was delivered. you can see this here
2. Electron recommends using MessagePorts to communicate between two windows - one can be hidden - (instead of BroadcastChannels, which is what we use now). The general idea is to pass one port created from the mainProcess to each window's preload and then pass that port to the renderProcess of each window, allowing to communicate with each other. They use window listeners instead of function calls to communicate, and they seem to be non-blocking. you can see that here.

I don't see the advantage of using MessagePorts instead of BroadcastChannels at a glance, but it seems that communicating between windows comes with a performance penalty. An alternative proposed on this article (which also serves as a nice rundown of possibilities), is to fork a process from the mainProcess and communicate to that forked process from the renderProcess using ipc.

[tomasciccola]

Also, using two windows - one hidden - since to be a common practice in electron since most IPC documentation comes with examples for this case, naming the hidden window worker

[gmaclennan]

First of all, my apologies that so much of this is poorly documented and a bit of a mess. A lot of the IPC layer was rewritten in https://github.com/digidem/mapeo-desktop/pull/465 but this was only half done (just enough to get everything working without bugs), and as a result the code is a bit of a mix between historical code and newer stuff, and there is still a lot of cleanup needed. See https://github.com/digidem/mapeo-desktop/pull/465 for some more details about the architecture.

The main reasoning for running multiple background processes (which are basically hidden windows) is that ipc between renderProcess and mainProcess is blocking, so if a request to a long running task is made from the UI to the mainProcess, it will block until it gets a response.

This is not entirely true. The actual reason is that the main process itself is blocking - any long-running code in the background process will "lock up" the UI, because with electron things like mouse interaction actually goes through the main process. We were running things like Mapeo Core indexing on the main process and it was blocking user interaction in the render process. This is very counter-intuitive!

We did originally use node-ipc (although an earlier version using [electron-rabbit[(https://github.com/okdistribute/electron-rabbit)), and we forked processes. However these forked processes were hard to manage and often left "dangling" when the app quit or had errors. We decided to follow the electron recommended way which says:

For long running CPU-heavy tasks, make use of worker threads, consider moving them to the BrowserWindow, or (as a last resort) spawn a dedicated process.

We chose the BrowserWindow because it allows for direct communication with the main render process window (via MessagePorts and BroadcastChannel). With worker threads we would need a more complicated IPC that went via the main process. And spawning a dedicated process led to the challenges I mentioned above.

@tomasciccola wrote:

Electron recommends using MessagePorts to communicate between two windows - one can be hidden - (instead of BroadcastChannels, which is what we use now)

We actually use MessagePorts right now. The code for this is still a bit scattered. Some of that is necessary because of the electron model, but also we have too many layers of extraction and API layers on top of APIs, because of historical reasons. The message ports are set up in https://github.com/digidem/mapeo-desktop/blob/master/src/renderer/index-preload.js#L25, and we use electron IPC to send the ports to the background processes. We use electron IPC for negotiating startup and stopping of background processes, but actual communication at runtime is via message ports.

The abstraction for managing these background processes (including starting and stopping associated with app reloads and restarts) is here: https://github.com/digidem/mapeo-desktop/blob/master/src/main/background-process.js and also https://github.com/digidem/mapeo-desktop/blob/master/src/background/index.js. This sets up the MessagePorts (via MessageChannel API).

The BroadcastChannels are using the same underlying tech as MessagePorts, but have a different use-case. They are used for broadcasting messages to all listeners. We currently use this in many places where MessagePorts might make more sense, but they were an easy drop-in replacement for some of the previous node-ipc code that avoided even further refactoring.

--

So much of this code can be simplified and completely removed with a cleanup of the whole IPC layer. However I think some of the existing code which handles startup and stopping of background processes is important, because it handles a lot of edge-cases with coordinating both process startup and reloads of the render window (which gets very complicated). I think running the background code (Mapeo Core, Mapeo Mapserver) in a background window, and communicating with MessagePorts and/or Broadcast channels continues to be the most efficient and easiest to maintain option.

I have a few (many) ideas for how we can integrate rpc-reflector, the new Mapeo Core API, and the current background process model, and I think it could greatly simplify a whole bunch of the code that we have here. Will try to sketch out some more ideas, but also would maybe make sense to jump on a call about it.

[tomasciccola]

Thanks so much for this follow up gregor. It clears some stuff up. For what I can see we're basically following electron best practices and also we've already tried using ipc and a forked process but it didn't turn out good. So I think the current architecture makes sense. Room for improvement seems to lie on better documentation, typing the client api and incorporating rpc-reflector as an intermediate layer.

[tomasciccola]

Takeaways after this research:

• In the current mapeo-desktop, the backend (mapeo-core) is run on a hidden window and the communication goes through a MessagePort.
• Using MessagePort is a best practice so this shouldn't change
• Instead of running the backend in a hidden window, we could run it on a utilityProcess (which is a forked nodejs process with message ports enabled, see this)
• There's also communication using BroadcastChannels which are used to communicate with a printer server and map-server. This has somewhat a complicated arch which is hard to know if it can be improved
• Improvements on this bridging layer (between front and back) lies on mostly having better documentation and typing.
• The communication between the backend and the frontend can be solved/improved with rpc-reflector.
• To be able to integrate rpc-reflector in mapeo-desktop, we need a bundler to polyfill nodejs stuff for the frontend (example); the alternative to that would be disabling the nodejs sandbox, but there are security concerns with this... I'm gonna mark this as done