-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
electron: fix backend forking #7386
Conversation
4445668
to
2f71ca7
Compare
Most changes are just related to debugging, as I tried to make sure that we could still debug code in both processes (electron main + node). The fork-related changes are the following: |
Why don't keep dev mode and make a minimal change? |
@akosyakov It will make Electron APIs fail right away in the backend when working in our repo. Before, we used to be able to require electron but it would fail once packaged. Just feels more robust to be closer to what runs in "production". |
I think it only makes debugging harder and does not make it close to production. You have to bundle to be closer to production. I think PR should be simpler and don't try to solve unrelated issues. |
I stand by the fact that it will make Electron APIs fail on the backend this way. |
It was not a problem so far, was it? There other means to prevent it as linting rules without complicating debugging setup. We have docs for such setups already which reused for all generated products and extensions. How are we going to update everything now? |
And now it won't ever! But if you really feel strongly about separating electron main process and backend process making it too hard for developers to work with, I can make the PR even more minimal, I just don't think we should strive for absolutely minimal changes, but rather reasonable changes. Please confirm that you really want to keep the old logic, for me to be sure. |
I mean that all products and extensions are relying on it for debugging Electron right now: https://github.com/theia-ide/generator-theia-extension/blob/master/templates/launch.json#L30-L58 Including docs in this repo: https://github.com/eclipse-theia/theia/blob/master/doc/Developing.md#debug-the-electron-examples-backend That's for sure not a trivial change without consequences.
Everybody figure out very fast that electron API does not work in the backend by plainly running from sources. We run in dev mode only for debugging, not running from the command line. |
The internal documentation can be updated, the generator is annoying indeed but it doesn't feel complete either. If anything, this makes me think that the generator package should be inside the theia repository itself, so that it is easier to align with changes we make. If we want to also hide implementation details regarding debugging from client POV, we could also make a VS Code extension contributing a I will bring back old behavior based on what you pointed out. |
It is not about generator. It is that debugging setup in the Arduino Pro IDE won't work anymore and all other products out there, including 3rd party Theia extensions, workshops materials and so on.
Thank you. |
But moving the generator sounds like a good idea. Could you file a proposal issue to see whether it is alright with everybody? I was thinking to change it to theia generator instead of only extension generator, so it can generate different layouts, i.e. to support Theia extension development, VS Code extension development against Theia or a hybrid approach, maybe also only applications. But it should be another issue and discussion. |
2f71ca7
to
b55558e
Compare
Updated the PR. |
@marechal-p thank you Paul! May I ask you to add comments to
I'm pretty sure that it was discussed before It would be document it this time that we won't wonder about it next year again. |
@kittaakos Could you have a look please? I agree with @marechal-p that running the express server in the electron main thread is not a good idea. I'm pretty sure we discussed it with @svenefftinge long time ago and decided to run in a separate process to avoid compromising performance of electron. |
I have checked the changes, thank you! I would leave it as is, the |
@kittaakos What do you mean? |
@akosyakov Did not see this bit earlier, but we do run in "devMode" when running from sources, not just when debugging. |
b55558e
to
4e05f0b
Compare
@akosyakov Added comments. |
@marechal-p I thought it is based on --no-cluster option meaning that electron main and express server should run in the same process. If it is based on something else you will need to dig why it was introduced and whether it is still relevant and whether it can be changed without affecting setups of downstream projects. |
If I remove it, 2 processes will be started when running from sources, leading to debugging issues like you mentioned before. @thegecko has a requirement that might make us reconsider spawning that process or not btw, see #7361 (comment) |
I think if you still respect |
97e5e66
to
2db3b50
Compare
@akosyakov Made the Electron generated code handle this But this made me see that arguments were not passed to the forked backend. I was able to confirm using theia-apps/theia-electron in a bundled application that the backend could not see any argument passed from the command line. This commit fixes it, although I could make it a second commit if someone wants it that way. |
When running a bundled application, the backend is forked. But any command line parameter passed to the Electron application isn't passed down to the backend. This commit makes sure that relevant arguments are passed to the forked backend process. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
When forking, Electron 4 forgets to set `process.versions.electron` in the sub-process, making some packages wrongly guess their environment. This commit manually patches this variable. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
2db3b50
to
805b7e0
Compare
Broken-up the PR into smaller commits. |
805b7e0
to
47c3032
Compare
@kittaakos Could you elaborate why patching is not good? Does not it fix all downstreams or you think it is not a common problem? @marechal-p maybe you move other changes to a new PR that they can be merged. I don't want to approve the patching because I am not so familiar with Electron products and Akos can know something what we overlook. |
I did not say, it is not good. It is a hack. I can do the same, in fact, I already have a workaround for the problem. |
Ok, i don't understand whether it fixes the issue or not and whether it helps anyone. Also don't know how to test it. |
47c3032
to
492db2b
Compare
WDYM? What would people do that I should not do here?
It is a bug, that we manually fix. At worst, this is monkey patching. But to argument in favor of doing this:
Tell me if you see anything else that would make this patching incomplete. |
@akosyakov I have updated the |
dev-packages/application-manager/src/generator/frontend-generator.ts
Outdated
Show resolved
Hide resolved
Thanks for preparing it, @marechal-p. I am going to verify your changes on a bundled electron app I work on and get back to you this week. Bear with me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- When forking, Electron 4 forgets to set
process.versions.electron
in
the sub-process, making some packages wrongly guess their environment.
This commit manually patches this variable.
Verified; it works as expected 👍
2. The generated application code also used to not fork the backend when
working from sources. Running the backend in the Electron main process
was only meant to help with debugging. This commit makes the generated
code fork the backend unless a--no-cluster
flag is specified.
Did you miss this feature or caused any issues so far? If no, let's leave as is.
3. When running a bundled application, the backend was forked. But any
command line parameter passed to the Electron application
I do not mind this change but in reality, no one will release a product and tell the users, start the app from a terminal with the following args.
3. If you want the generated code to not fork the backend in a sub-process,
you can either pass a--no-cluster
flag to your application, or set
THEIA_ELECTRON_NO_BACKEND_FORK=1
environment variable.
Let's just not do 2.
.
I have verified 1
: process.versions.electron
. I do not mind if we change this.
For sure, I won't use 2.
and 3.
in the near future, but if you want this modification, I have no objections against them.
@marechal-p, next time let's spend the same amount of effort we and especially you have spent on this issue with the electron 8 migration. Now, we have this process.version.electron
fix, which has not caused a problem for anyone so far, except with grpc
.
I do, thanks for the review! |
The generated application code is not forking the backend when working from sources. Running the backend in the Electron main process was only meant to help with debugging. This commit makes the generated code fork the backend unless a `--no-cluster` flag is specified. Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
492db2b
to
1714222
Compare
This was a quick fix, but I would be happy to help with Electron 8 once we get there! |
What it does
This PR fixes 3 issues:
When forking, Electron 4 forgets to set
process.versions.electron
inthe sub-process, making some packages wrongly guess their environment.
This commit manually patches this variable.
The generated application code also used to not fork the backend when
working from sources. Running the backend in the Electron main process
was only meant to help with debugging. This commit makes the generated
code fork the backend unless a
--no-cluster
flag is specified.When running a bundled application, the backend was forked. But any
command line parameter passed to the Electron application wasn't passed
down to the backend. This commit makes sure that relevant arguments
are passed to the forked backend process.
If you want the generated code to not fork the backend in a sub-process,
you can either pass a
--no-cluster
flag to your application, or setTHEIA_ELECTRON_NO_BACKEND_FORK=1
environment variable.Closes: #7361
Fixes: #7358
Fixes: #4288
Fixes: #4875
How to test
Add
grpc
as a dependency, and addrequire('grpc')
inexamples/electron/src-gen/backend/server.js
. If you place the require before the patching of theprocess.versions.electron
it should not work, but it should work if you put the require after.note:
grpc
might need to be configured to run with Electron. What I did was:cd node_modules/grpc yarn node-pre-gyp --runtime=electron --target=4.2.12 install
Review checklist
Reminder for reviewers