Vscode: Extension Host Failure - Error interaction with MATLAB code on macOS Catalina
Version: 1.47.3
Commit: 91899dcef7b8110878ea59626991a18c8a6a1b3e
Date: 2020-07-23T13:08:29.692Z (2 wks ago)
Electron: 7.3.2
Chrome: 78.0.3904.130
Node.js: 12.8.1
V8: 7.8.279.23-electron.0
OS: Darwin x64 19.6.0
_Note: Tested Insiders and it suffers from the same issue._
Steps to Reproduce:
- Have MATLAB 2018 or later installed on your system.
- Open VS Code via
/Applications/Visual Studio Code.app(Launchpad, Finder, or Spotlight) - Observe Extension Host crash with the following error:
dyld: Symbol not found: _mecab_get_feature Referenced from: /System/Library/PrivateFrameworks/CoreNLP.framework/Versions/A/CoreNLP Expected in: /Applications/MATLAB/MATLAB_Runtime/v95/bin/maci64/libmecab.dylib in /System/Library/PrivateFrameworks/CoreNLP.framework/Versions/A/CoreNLP
Does this issue occur when all extensions are disabled?: No
It should be noted that the issue does not occur when launched via Terminal at all (even when extensions are enabled). It appears to be something related to the app wrapper.
Workaround:
- Open via Terminal using
/Applications/Visual\ Studio\ Code.app/Contents/Resources/app/bin/codeinstead of launching using the app launcher itself.
mko
All 29 comments
So this only happens when extensions are enabled? Which one causes this?
mjbvz
on 13 Aug 2020
Closing as likely caused by an extension based on the description
mjbvz
on 21 Aug 2020
@mjbvz -- Apologies for not replying sooner. Just saw the Closed notice. This is not caused because of an extension. It is caused by the Application Wrapper within /Applications when MATLAB is installed. It crashes the Extension Host even with all installed extensions disabled.
mko
on 21 Aug 2020
@deepak1556 This looks similar to #103057
@mko Have you by any chance customized your DYLD_LIBRARY_PATH ?
alexdima
on 24 Aug 2020
@alexdima I can’t check right now. I’ll check when I’m able to, but I know that MATLAB requires its compiler and related libraries to be added to the DYLD_LIBRARY_PATH in order to run without having to append 100+ characters to every command. This is not a “package-specific error unrelated to VSCode” though. VSCode, unlike any other software on the system that accesses a PATH variable has a failure case caused when a developer does an extremely common developer thing: that is, extends their path. To dismiss it as just a package issue instead of doing the work to find the bug in your own code to prevent the Extension Host from crashing during this use case means you block off a significant segment of users who cannot change the way your system interacts with the rest of theirs.
mko
on 24 Aug 2020
Here is what I could find about DYLD_LIBRARY_PATH -- https://stackoverflow.com/a/3172515 -- I personally have never used that.
In my opinion, would it not make sense to define DYLD_LIBRARY_PATH only when invoking the MATLAB compiler and not globally?
VS Code is built with Electron, which is built with Chromium. Perhaps you define that environmental variable, and that leads to Electron/Chromium respecting it (maybe it is an OS thing and outside of Electron/Chromium's control?) and attempting to load a symbol from a dynamic library found in the path you have defined for MATLAB? Suppose Electron/Chromium needs to load protobuf and now it finds protobuf in the path you have defined for MATLAB. It might or might not work depending on if protobuf is exactly at the same version / has the same symbols as the one expected by Electron/Chromium.
Perhaps @deepak1556 has more insights, I'm sorry this is as much as my knowledge on the subject goes.
alexdima
on 24 Aug 2020
Looking at the past issues filed and why only VSCode is affected, not other electron apps, I think I can conclude where the issue happens to be. When a GUI app is launched on mac, it would be usually via the finder or open command (launch services basically isolating the app launch) and not from the terminal which would mean the app is launched as a subprocess of it, getting the same set of environment configuration for all its process. Chromium has its own process launch management, so the renderer and other process launched don't get affected with this but since we fork the extension host process from the renderer, meaning we use the same helper executable as the renderer but with the shell environments copied to it, it eventually leads to this sort of incorrect symbol load.
This should be fixed once we land https://github.com/microsoft/vscode/pull/104470 but as a short term fix we can remove the DYLD_LIBRARY_PATH from the launch wrapper env https://github.com/microsoft/vscode/blob/master/src/vs/code/node/cli.ts#L320 so that it doesn't propagate. Thoughts @alexdima ?
deepak1556
on 24 Aug 2020
@mko How exactly have you configured DYLD_LIBRARY_PATH ? I would like to attempt to reproduce the crash such that we can come up with a potential fix.
@deepak1556 I don't think cli.ts plays any part in forking the extension host process. The fork happens here -- https://github.com/microsoft/vscode/blob/bb624e94f2a9c01b19e9ad89303b157df87ce93b/src/vs/workbench/services/extensions/electron-browser/localProcessExtensionHost.ts#L159-L167 -- We could try to unset DYLD_LIBRARY_PATH there, but before doing that change I would like to be able to reproduce the problem.
alexdima
on 24 Aug 2020
Doesn't cli.ts launch the app as a child process, so the env variables it launches gets propagated all the way down because of process.env. For example.
$ export ABC=1
$ code-insiders vscode/
$ check process.env.ABC in renderer process
$ check process.env.ABC in extension host process.
thats why the author didn't see the crash when launching without the cli wrapper
deepak1556
on 24 Aug 2020
DYLD_LIBRARY_PATH is set as an environment variable in the bash profile with the following, per MATLAB Compiler Runtime instructions:
DYLD_LIBRARY_PATH=/Applications/MATLAB/MATLAB_Runtime/v95/runtime/maci64:/Applications/MATLAB/MATLAB_Runtime/v95/sys/os/maci64:/Applications/MATLAB/MATLAB_Runtime/v95/bin/maci64:/Applications/MATLAB/MATLAB_Runtime/v95/extern/bin/maci64:$DYLD_LIBRARY_PATH
@alexdima: I totally agree that it would make more sense to only define the updated path when MATLAB is being invoked, but that's not possible with the way that MATLAB compiled binaries work. In both our development environment and several third-party packages we have used, the MCR binaries get invoked through processes where the developer typically does not have the ability to define an environment variable. The issue that I'm seeing though is that there are no "override" symbols between the system and MATLAB (i.e. no protobuf or libjpeg or similar symbols that would be expected to have already been provided elsewhere).
mko
on 24 Aug 2020
As I noted previously, I'm able to get extensions working as long as I launch code directly. I doubt this affects many people, so I'm happy to provide whatever I can in debugging help to solve it the right way. Let me know if there's anything else I can provide (debug logs, bin contents, etc), and if the solution is to just wait until #104470 lands, I think that's probably reasonable as well (as it does look like it may fix the problem based on what I read there).
mko
on 24 Aug 2020
cc @bpasero
alexdima
on 25 Aug 2020
@deepak1556 @alexdima note that on Linux & macOS if we detect that VSCode is run from e.g. the dock (and not from the CLI - by checking for VSCODE_CLI environment variable), we use the method getShellEnvironment method to spawn a shell to get environment variables from the shell back into the process.env. The rationale is to get development related environment variables into VSCode, e.g. to support task execution.
If we want to filter DYLD_LIBRARY_PATH, it would also have to be added to that place.
bpasero
on 25 Aug 2020
Thanks @bpasero that explains why the env variable was carried over to the renderer, my understanding was incorrect.
Just remembered a past issue where there was a valid use case for DYLD_LIBRARY_PATH in tasks https://github.com/microsoft/vscode/issues/88306 , do you think this use case will continue to work if we filter it from shell env ?
This issue has to wait until https://github.com/microsoft/vscode/pull/104470 lands, the change ensures app is not launched via shell, so VSCODE_CLI wouldn't be effective. This is something that was missed, I will add it to the PR discussion.
deepak1556
on 25 Aug 2020
Just remembered a past issue where there was a valid use case for DYLD_LIBRARY_PATH in tasks #88306 , do you think this use case will continue to work if we filter it from shell env ?
My understanding is that DYLD_LIBRARY_PATH might have negative impact on VSCode itself, so a solution that allows to use DYLD_LIBRARY_PATH in tasks (or anywhere else in extensions code) should support this variable exclusively for that child process, right? In other words, I do not think it is a good idea to support DYLD_LIBRARY_PATH in this scenario by adding it to the environment of VSCode itself, it should only apply to the processes that are executed that really need it.
And to clarify: If we decide to filter DYLD_LIBRARY_PATH, it would be everywhere, not just from that getShellEnvironment call.
Maybe one approach would be to filter DYLD_LIBRARY_PATH by renaming it to VSCODE_DYLD_LIBRARY_PATH and then ensuring to put it back in selected places where users really want it?
This issue has to wait until #104470 lands, the change ensures app is not launched via shell, so VSCODE_CLI wouldn't be effective. This is something that was missed, I will add it to the PR discussion.
Not sure I follow what you said, we have to ensure VSCODE_CLI is present for any invocation of VSCode from the command line.
bpasero
on 26 Aug 2020
My understanding is that DYLD_LIBRARY_PATH might have negative impact on VSCode itself, so a solution that allows to use DYLD_LIBRARY_PATH in tasks (or anywhere else in extensions code) should support this variable exclusively for that child process, right?
Yup thats correct
Maybe one approach would be to filter DYLD_LIBRARY_PATH by renaming it to VSCODE_DYLD_LIBRARY_PATH and then ensuring to put it back in selected places where users really want it?
Sounds good, but I am not familiar with the task execution system to verify if this will work fine. Atleast would be good to verify with the sample from https://github.com/microsoft/vscode/issues/88306 . Also if there are other use cases, I would like to capture it in this issue.
Not sure I follow what you said, we have to ensure VSCODE_CLI is present for any invocation of VSCode from the command line.
That won't be true with https://github.com/microsoft/vscode/pull/104470 , since the app won't be invoked from the shell. We are trying to emulate the behavior of opening from finder. I just remembered that we missed VSCODE_CLI context in that PR, so wanted to capture it. I haven't thought about the way forward, we can continue the discussion in the associated issue.
deepak1556
on 26 Aug 2020
Sounds good, but I am not familiar with the task execution system to verify if this will work fine. Atleast would be good to verify with the sample from #88306 . Also if there are other use cases, I would like to capture it in this issue.
👍
That won't be true with #104470
To clarify: if we switch to open command, none of the shell process environment or anything we configure in our CLI scripts ends up in the process environment of the VSCode window? That would be pretty bad and we then need an alternative for how to getting these variables in. Even when running out of sources, code.sh defines many variables:
bpasero
on 26 Aug 2020
To clarify: if we switch to open command, none of the shell process environment or anything we configure in our CLI scripts ends up in the process environment of the VSCode window? That would be pretty bad and we then need an alternative for how to getting these variables in. Even when running out of sources, code.sh defines many variables:
yeah thats correct, the only way I see now is to pass them via command line flags and modify process.env in the app, should figure a good solution for this.
Even when running out of sources, code.sh defines many variables:
we are not gonna change the out of sources cli wrapper, so this one should not be an issue.
deepak1556
on 26 Aug 2020
@bpasero @deepak1556 I'm not 100% sure I understood your discussion above. So what is the final proposal here? Is this something that needs to be done only for the extension host process or is this something that needs to be done for the entire workbench? What is the final proposed change?
alexdima
on 30 Aug 2020
ping @bpasero @deepak1556
alexdima
on 3 Sep 2020
I talked with @deepak1556 in our Monday call and we think that removing this variable here should be fine:
This means that the variable will never reach extensions, but my understanding is that people only want it for task execution which currently does not happen in the extension host (iirc).
bpasero
on 3 Sep 2020
Sorry missed this, thanks @bpasero @alexdima , yeah this should fix the issue.
deepak1556
on 3 Sep 2020
@mko can you confirm if this issue is fixed with latest insiders https://code.visualstudio.com/insiders/
deepak1556
on 4 Sep 2020
I cannot reproduce the original crash even when I set export DYLD_LIBRARY_PATH=<path>, I see no extension host crashing. What are the steps?
bpasero
on 4 Sep 2020
@bpasero From my understanding, setting the environmental variable is one part of the problem. The crash appears if there is a matching library found at that folder location. So for example, our process wants to load protobuf and then that folder has protobuf, but at a version that is not compatible, etc.
alexdima
on 4 Sep 2020
Oh ok, then maybe best to let author verify.
bpasero
on 4 Sep 2020
I’ll try verifying it this afternoon. Thanks
On Sep 4, 2020, at 4:12 AM, Benjamin Pasero notifications@github.com wrote:
Oh ok, then maybe best to let author verify.—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or unsubscribe.
mko
on 4 Sep 2020
Sorry -- ended up not getting to verify until today, but I can verify this is solved.
mko
on 8 Sep 2020
cc: @bpasero -- Verified
mko
on 8 Sep 2020
Related issues
omidgolparvar
·
3Comments
sirius1024
·
3Comments
biij5698
·
3Comments
VitorLuizC
·
3Comments
curtw
·
3Comments