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

getShellEnvironment might never finish #24435

Closed
bpasero opened this issue Apr 10, 2017 · 5 comments
Closed

getShellEnvironment might never finish #24435

bpasero opened this issue Apr 10, 2017 · 5 comments
Assignees
Labels
debt Code quality issues

Comments

@bpasero
Copy link
Member

bpasero commented Apr 10, 2017

During load of the window we actually stop any loading when we call into process.lazyEnv and if there is any error during this call, we just get an empty window presented (as reported by many users in #23639).

There are multiple problems:

  • there is no logging or notification going on when the resolution of the shell environment takes long or fails, making it hard for us to find out about these issues (e.g. the window is just blank and dev tools shows nothing)
  • we seem to not resolve any environment on Windows and also not when process.env['VSCODE_CLI'] is set so I would expect we can skip this call on the renderer side and save us the inter process communication on startup in those cases
@bpasero bpasero added debt Code quality issues perf labels Apr 10, 2017
@jrieken jrieken removed the perf label Apr 10, 2017
@jrieken
Copy link
Member

jrieken commented Apr 10, 2017

there is no logging or notification going on when the resolution of the shell environment takes long or fails, making it hard for us to find out about these issues (e.g. the window is just blank and dev tools shows nothing)

Yes, ideally the resolving of the environment finishes without errors and in a timely fashion. The current implementation is done like that because we statically reference process.env in our code and cannot do that unless we block loading code which has negative performance implications.

we seem to not resolve any environment on Windows and also not when process.env['VSCODE_CLI'] is set so I would expect we can skip this call on the renderer side and save us the inter process communication on startup in those cases

We don't need to.

@jrieken
Copy link
Member

jrieken commented Apr 10, 2017

The fix will be to log an error to the console and to simply continue with a potential incomplete environment

@jrieken
Copy link
Member

jrieken commented Apr 10, 2017

The lazyEnv-promise behaves actually like that. It's getShellEnvironment that doesn't finish

getShellEnvironment().then(shellEnv => {
	win.webContents.send('vscode:acceptShellEnv', shellEnv);
}, err => {
	win.webContents.send('vscode:acceptShellEnv', {});
	console.error('Error fetching shell env', err);
});

@jrieken jrieken changed the title process.lazyEnv is dangerous getShellEnvironment might never finish Apr 10, 2017
@joaomoreno joaomoreno added this to the April 2017 milestone Apr 10, 2017
@jrieken jrieken reopened this May 11, 2017
@jrieken jrieken removed their assignment May 11, 2017
@jrieken jrieken removed this from the April 2017 milestone May 11, 2017
@jrieken
Copy link
Member

jrieken commented May 11, 2017

Timeout apparently wasn't the way to go

@jrieken jrieken reopened this May 11, 2017
@joaomoreno
Copy link
Member

joaomoreno commented May 11, 2017

Increased the timeout to 10s. Let's add some perf numbers to know exactly how long does the renderer wait for that env to come.

@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
debt Code quality issues
Projects
None yet
Development

No branches or pull requests

3 participants