Do not fail interpreter discovery if accessing Windows registry fails. #13369

karrtikr · 2020-08-10T17:04:39Z

For #12962

sonarqubecloud · 2020-08-10T17:07:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
0 Code Smells

No Coverage information
1.0% Duplication

codecov-commenter · 2020-08-10T17:14:31Z

Codecov Report

Merging #13369 into master will decrease coverage by 0.06%.
The diff coverage is 16.66%.

@@            Coverage Diff             @@
##           master   #13369      +/-   ##
==========================================
- Coverage   59.75%   59.68%   -0.07%     
==========================================
  Files         670      670              
  Lines       37303    37347      +44     
  Branches     5296     5306      +10     
==========================================
+ Hits        22290    22291       +1     
- Misses      13878    13919      +41     
- Partials     1135     1137       +2

Impacted Files	Coverage Δ
src/client/common/platform/registry.ts	`27.65% <12.50%> (-1.61%)`	⬇️
...covery/locators/services/windowsRegistryService.ts	`89.47% <25.00%> (-1.84%)`	⬇️
src/client/common/utils/platform.ts	`64.70% <0.00%> (-11.77%)`	⬇️
src/client/datascience/crossProcessLock.ts	`79.41% <0.00%> (-11.77%)`	⬇️
...ient/datascience/editor-integration/codewatcher.ts	`68.03% <0.00%> (-2.59%)`	⬇️
src/client/linters/pydocstyle.ts	`86.66% <0.00%> (-2.23%)`	⬇️
src/client/datascience/debugLocationTracker.ts	`76.56% <0.00%> (-1.57%)`	⬇️
src/client/common/process/proc.ts	`14.49% <0.00%> (-0.73%)`	⬇️
src/datascience-ui/common/index.ts	`95.72% <0.00%> (-0.15%)`	⬇️
src/client/datascience/commands/commandRegistry.ts	`34.50% <0.00%> (-0.03%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9df5ef4...776a5ad. Read the comment docs.

karthiknadig · 2020-08-10T19:01:46Z

src/client/common/platform/registry.ts

-        return getRegistryKeys({ hive: translateHive(hive)!, arch: translateArchitecture(arch), key });
+        return getRegistryKeys({ hive: translateHive(hive)!, arch: translateArchitecture(arch), key }).catch((ex) => {
+            traceError('Fetching keys from windows registry resulted in an error', ex);
+            return [];


Look at the getRegistryKeys implementation. We already return empty array. I don't think we will hit this case.

vscode-python/src/client/common/platform/registry.ts

Line 52 in 61b179b

return resolve([]);

vscode-python/src/client/common/platform/registry.ts

Lines 54 to 66 in 776a5ad

async function getRegistryKeys(options: Options): Promise<string[]> {

// tslint:disable-next-line:no-require-imports

const Registry = require('winreg') as typeof import('winreg');

// https://github.com/python/peps/blob/master/pep-0514.txt#L85

return new Promise<string[]>((resolve) => {

new Registry(options).keys((error, result) => {

if (error || !Array.isArray(result)) {

return resolve([]);

}

resolve(result.filter((item) => typeof item.key === 'string').map((item) => item.key));

});

});

}

I think you're assuming that new Registry(options).keys at line 59 does not throw any errors and respects the callback. I checked the error trace user uploaded,

console.ts:137 [Extension Host] Error Python Extension: 2020-07-15 14:50:32: Get Interpreters, Class name = m, completed in 9ms, has a falsy return value, Arg 1: <Uri:c:\Users\chmay\source\mkl\mkl_email_service>, Arg 2: {"onSuggestion":true}, Return Value: undefined Error: spawn EPERM at ChildProcess.spawn (internal/child_process.js:394:11) at spawn (child_process.js:553:9) at y.keys (c:\Users\chmay\.vscode\extensions\ms-python.python-2020.6.91350\out\client\extension.js:1:621980) at c:\Users\chmay\.vscode\extensions\ms-python.python-2020.6.91350\out\client\extension.js:1:617508 at new Promise (<anonymous>) at c:\Users\chmay\.vscode\extensions\ms-python.python-2020.6.91350\out\client\extension.js:1:617483 at c.getKeys (c:\Users\chmay\.vscode\extensions\ms-python.python-2020.6.91350\out\client\extension.js:1:617615) at b.getCompanies (c:\Users\chmay\.vscode\extensions\ms-python.python-2020.6.91350\out\client\extension.js:48:796340) at b.getInterpretersFromRegistry (c:\Users\chmay\.vscode\extensions\ms-python.python-2020.6.91350\out\client\extension.js:48:795831)

It fails at y.keys in the extension code, which is new Registry(options).keys

I see. Go it.

karthiknadig · 2020-08-10T19:02:23Z

src/client/common/platform/registry.ts

+        return getRegistryValue({ hive: translateHive(hive)!, arch: translateArchitecture(arch), key }, name).catch(
+            (ex) => {
+                traceError('Fetching key value from windows registry resulted in an error', ex);
+                return undefined;


Same as above.

karrtikr · 2020-08-10T21:02:26Z

src/client/pythonEnvironments/discovery/locators/services/windowsRegistryService.ts

@@ -47,7 +47,12 @@ export class WindowsRegistryService extends CacheableLocatorService {
    // tslint:disable-next-line:no-empty
    public dispose() {}
    protected async getInterpretersImplementation(_resource?: Uri): Promise<PythonInterpreter[]> {
-        return this.platform.isWindows ? this.getInterpretersFromRegistry() : [];
+        return this.platform.isWindows


I have put this just to make sure we fix the bug. We should probably have done this with all locators.

ericsnowcurrently

LGTM

This feels more like a bandaid than a full fix, but I understand the constraints here.

Kartik Raj added 2 commits August 10, 2020 08:41

Do not fail discovery if accessing Windows registry fails

708dbfc

News entry

776a5ad

karrtikr requested review from ericsnowcurrently and karthiknadig August 10, 2020 17:58

karthiknadig reviewed Aug 10, 2020

View reviewed changes

karrtikr commented Aug 10, 2020

View reviewed changes

karthiknadig approved these changes Aug 10, 2020

View reviewed changes

ericsnowcurrently approved these changes Aug 11, 2020

View reviewed changes

karrtikr merged commit e044a04 into microsoft:master Aug 11, 2020

karrtikr deleted the registry branch August 11, 2020 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not fail interpreter discovery if accessing Windows registry fails. #13369

Do not fail interpreter discovery if accessing Windows registry fails. #13369

karrtikr commented Aug 10, 2020 •

edited

Loading

sonarqubecloud bot commented Aug 10, 2020

codecov-commenter commented Aug 10, 2020 •

edited

Loading

karthiknadig Aug 10, 2020

karrtikr Aug 10, 2020 •

edited

Loading

karthiknadig Aug 10, 2020

karthiknadig Aug 10, 2020

karrtikr Aug 10, 2020

ericsnowcurrently left a comment

	async function getRegistryKeys(options: Options): Promise<string[]> {
	// tslint:disable-next-line:no-require-imports
	const Registry = require('winreg') as typeof import('winreg');
	// https://github.com/python/peps/blob/master/pep-0514.txt#L85
	return new Promise<string[]>((resolve) => {
	new Registry(options).keys((error, result) => {
	if (error \|\| !Array.isArray(result)) {
	return resolve([]);
	}
	resolve(result.filter((item) => typeof item.key === 'string').map((item) => item.key));
	});
	});
	}

Do not fail interpreter discovery if accessing Windows registry fails. #13369

Do not fail interpreter discovery if accessing Windows registry fails. #13369

Conversation

karrtikr commented Aug 10, 2020 • edited Loading

sonarqubecloud bot commented Aug 10, 2020

codecov-commenter commented Aug 10, 2020 • edited Loading

Codecov Report

karthiknadig Aug 10, 2020

Choose a reason for hiding this comment

karrtikr Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

karthiknadig Aug 10, 2020

Choose a reason for hiding this comment

karthiknadig Aug 10, 2020

Choose a reason for hiding this comment

karrtikr Aug 10, 2020

Choose a reason for hiding this comment

ericsnowcurrently left a comment

Choose a reason for hiding this comment

karrtikr commented Aug 10, 2020 •

edited

Loading

codecov-commenter commented Aug 10, 2020 •

edited

Loading

karrtikr Aug 10, 2020 •

edited

Loading