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

Implement backend to electron IPC channels #10698

Closed
wants to merge 1 commit into from

Conversation

msujew
Copy link
Member

@msujew msujew commented Feb 2, 2022

What it does

Closes #2162

Uses a similar architecture to the existing Electron Main <-> Frontend communication layer over ipcRenderer and ipcMain. Contains additional changes to deal with the --no-cluster argument by using a special pipe object.

For testing purposes I included a TestConnection interface which returns the titles of all opened electron windows on startup to the backend server.

In a second version of this we could mirror the proposed classes/APIs here to also enable electron -> backend requests (this PR only enables backend -> electron requests), since we create only a single backend server for each electron main.

Note: My use case for this change is that I plan to add proxy support for all requests in Theia. In particular, requests from the backend running in Electron should try to resolve to the Chromium proxy (which automatically picks up the OS proxy) in case no proxy is specified using the apps' preferences. For this, we need some kind of way to communicate with the electron process from the backend server process.

How to test

  1. Start the Electron app.
  2. Search for Titles: in the console. The console should contain the title of the newly opened Electron window.
  3. Repeat with the --no-cluster argument enabled.

Review checklist

Reminder for reviewers

@msujew msujew added electron issues related to the electron target messaging issues related to messaging labels Feb 2, 2022
@msujew msujew requested a review from paul-marechal February 9, 2022 16:26
Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very preliminary comments only. @paul-marechal will certainly have more to say.

}

protected closeChannels(): void {
for (const channel of Array.from(this.channels.values())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (const channel of Array.from(this.channels.values())) {
for (const channel of this.channels.values()) {

This should already be iterable.

@@ -0,0 +1,59 @@
/********************************************************************************
* Copyright (C) 2012 TypeFox and others.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Copyright (C) 2012 TypeFox and others.
* Copyright (C) 2022 TypeFox and others.

Perhaps?

protected handleMessage(data: string): void {
try {
// Start parsing the message to extract the channel id and route
const message: WebSocketChannel.Message = JSON.parse(data.toString());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One point of the discussion re: performance in our communication layer is that conversions back and forth from string to JSON are relatively expensive. Will all messages be passing through here, or only some, or how will it fit into the refactor of the messaging layer currently underway? Is it expected that this will be one terminus of the messaging system that can delegate the parsed messages to handlers, or will further parsing be necessary?

@msujew
Copy link
Member Author

msujew commented Mar 8, 2022

@colin-grant-work Thanks for looking into this, but I think we discussed in one of our dev-meetings that I'll postpone this PR until Paul has finished his work on the messaging refactoring. I'll then have to rewrite most of this anyway ;)

I'll mark it as a draft PR for now.

@msujew msujew marked this pull request as draft March 8, 2022 10:52
@msujew msujew mentioned this pull request Mar 30, 2022
1 task
@msujew msujew mentioned this pull request Apr 19, 2022
1 task
@msujew
Copy link
Member Author

msujew commented May 30, 2022

Closing due to changes in IPC/Messaging. Will (possibly) continue working on this later.

@msujew msujew closed this May 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
electron issues related to the electron target messaging issues related to messaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to access electron features
2 participants