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.

Sign up for GitHub

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

Fix electron extension host #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix electron extension host #96

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 29 additions & 25 deletions packages/plugin-dev/src/node/hosted-instance-manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,11 @@ import * as cp from 'child_process';
import * as fs from '@theia/core/shared/fs-extra';
import * as net from 'net';
import * as path from 'path';
import * as os from 'os';
import URI from '@theia/core/lib/common/uri';
import { ContributionProvider } from '@theia/core/lib/common/contribution-provider';
import { HostedPluginUriPostProcessor, HostedPluginUriPostProcessorSymbolName } from './hosted-plugin-uri-postprocessor';
import { environment, isWindows } from '@theia/core';
import { isWindows } from '@theia/core';
import { FileUri } from '@theia/core/lib/common/file-uri';
import { LogType } from '@theia/plugin-ext/lib/common/types';
import { HostedPluginSupport } from '@theia/plugin-ext/lib/hosted/node/hosted-plugin';
Expand Down Expand Up @@ -240,23 +241,22 @@ export abstract class AbstractHostedInstanceManager implements HostedInstanceMan
}

protected async getStartCommand(port?: number, debugConfig?: PluginDebugConfiguration): Promise<string[]> {

if (!port) {
port = process.env.HOSTED_PLUGIN_PORT ?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried running the plugin a second time, but that failed. My suspicion is that we're trying to run a second instance with the same port. We need some algorithm to assign unique, free ports per instance.

Copy link
Author

Choose a reason for hiding this comment

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

Is it actually intended, that we will ever get a second instance? I just tested it in VSCode and there i will only get one extension host. If i start the same config again, while the old one is still running it will just kill the first debug session.

Of course the handling here could be improved, but i am not sure if we need to handle mutliple ports? WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, but something just not working without any indication why is simply a bug. If VS Code jumped out the Window, would you follow? ;-) There might be good reasons why we can't implement this, but then we need to catch it and inform the user. Is there a good reason we can't allocate a new random port?

Copy link
Author

Choose a reason for hiding this comment

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

I do not have any objections to support mutliple Hosted Plugin instances, but the current implementation seems like it was build under the assumption that only one will be possible (e.g. the cli parameter to specify one port, or the UI in the parent instance). As this would require additional changes, outside the scope of this PR, i would suggest to handle this in a follow up ticket. Does that sound good to you?

Number(process.env.HOSTED_PLUGIN_PORT) :
(debugConfig?.debugPort ? Number(debugConfig.debugPort) : DEFAULT_HOSTED_PLUGIN_PORT);
}
const processArguments = process.argv;
let command: string[];
if (environment.electron.is()) {
command = ['yarn', 'theia', 'start'];
} else {
command = processArguments.filter((arg, index, args) => {
// remove --port=X and --port X arguments if set
// remove --plugins arguments
if (arg.startsWith('--port') || args[index - 1] === '--port') {
return;
} else {
return arg;
}
const command = processArguments.filter((arg, index, args) => {
// remove --port=X and --port X arguments if set
// remove --plugins arguments
if (arg.startsWith('--port') || args[index - 1] === '--port') {
return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know you didn't invent this code, but it would be much clearer if we just returned true/false instead of arg and undefined. "truthiness" is bogus enough without us adding to the confusion. Also, it might be wrong: the empty string is falsy, so if we actually have an empty argument, it borks! 🤦

} else {
return arg;
}

});
}
});
if (process.env.HOSTED_PLUGIN_HOSTNAME) {
command.push('--hostname=' + process.env.HOSTED_PLUGIN_HOSTNAME);
}
Expand Down Expand Up @@ -365,18 +365,22 @@ export class NodeHostedPluginRunner extends AbstractHostedInstanceManager {
}
return options;
}

protected override async getStartCommand(port?: number, debugConfig?: PluginDebugConfiguration): Promise<string[]> {
if (!port) {
port = process.env.HOSTED_PLUGIN_PORT ?
Number(process.env.HOSTED_PLUGIN_PORT) :
(debugConfig?.debugPort ? Number(debugConfig.debugPort) : DEFAULT_HOSTED_PLUGIN_PORT);
}
return super.getStartCommand(port, debugConfig);
}
}

@injectable()
export class ElectronNodeHostedPluginRunner extends AbstractHostedInstanceManager {
protected override async getStartCommand(port?: number, debugConfig?: PluginDebugConfiguration): Promise<string[]> {
const command = await super.getStartCommand(port, debugConfig);
command.push(`--electronUserData="${this.getTempDir()}/theia-extension-host/${this.getTimestamp()}"`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO, we should track this directory and remove it once the hosted instance exits.

Copy link
Author

Choose a reason for hiding this comment

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

You mean, once the hosted instance is removed/terminated?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes.

return command;
}

protected getTempDir(): string {
const tempDir = os.tmpdir();
return process.platform === 'darwin' ? fs.realpathSync(tempDir) : tempDir;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could use a comment. Why the special case?

}

protected getTimestamp(): string {
return `${Math.round(new Date().getTime() / 1000)}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just move this into getTempDir()? Also: doesn't this limit the rate of temp dir creation to one per second? I would be more comfortable with a random directory + checking for existence.

}
}
Loading