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

Add extra logging regarding interpreter discovery #21639

Merged
merged 5 commits into from
Jul 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion src/client/common/process/rawProcessApis.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ export function plainExec(
const deferred = createDeferred<ExecutionResult<string>>();
const disposable: IDisposable = {
dispose: () => {
if (!proc.killed && !deferred.completed) {
if (!proc.killed) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also auto reject the promise to prevent dangling async operation?

Copy link
Author

Choose a reason for hiding this comment

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

Which promise? Asking as these are all sync operations.

Copy link
Member

Choose a reason for hiding this comment

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

the one that used in deferred

Copy link
Author

Choose a reason for hiding this comment

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

Ah, right.

Copy link
Author

Choose a reason for hiding this comment

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

Actually that's not necessary, as proc.once('close' is called on process killing which resolves or reject deferred based on certain conditions.

proc.kill();
}
},
Expand Down Expand Up @@ -156,10 +156,12 @@ export function plainExec(
deferred.resolve({ stdout, stderr });
}
internalDisposables.forEach((d) => d.dispose());
disposable.dispose();
Copy link
Author

Choose a reason for hiding this comment

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

@karthiknadig Hope forcefully killing processes after execution finishes is okay

Copy link
Member

Choose a reason for hiding this comment

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

This is technically not when execution is completed, but rather when dispose is called. But yes, it should be fine.

Copy link
Author

@karrtikr karrtikr Jul 17, 2023

Choose a reason for hiding this comment

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

This is executed on proc.once('close', which is run just after execution completes.

});
proc.once('error', (ex) => {
deferred.reject(ex);
internalDisposables.forEach((d) => d.dispose());
disposable.dispose();
});

return deferred.promise;
Expand Down
6 changes: 6 additions & 0 deletions src/client/common/utils/async.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,17 @@ class DeferredImpl<T> implements Deferred<T> {
}

public resolve(_value: T | PromiseLike<T>) {
if (this.completed) {
return;
}
this._resolve.apply(this.scope ? this.scope : this, [_value]);
this._resolved = true;
}

public reject(_reason?: string | Error | Record<string, unknown>) {
if (this.completed) {
return;
}
this._reject.apply(this.scope ? this.scope : this, [_reason]);
this._rejected = true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ export class ActiveStateLocator extends LazyResourceBasedLocator {
traceVerbose(`Couldn't locate the state binary.`);
return;
}
traceVerbose(`Searching for active state environments`);
const projects = await state.getProjects();
if (projects === undefined) {
traceVerbose(`Couldn't fetch State Tool projects.`);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ export class MicrosoftStoreLocator extends FSWatchingLocator {

protected doIterEnvs(): IPythonEnvsIterator<BasicEnvInfo> {
const iterator = async function* (kind: PythonEnvKind) {
traceVerbose('Searching for windows store envs');
const exes = await getMicrosoftStorePythonExes();
yield* exes.map(async (executablePath: string) => ({
kind,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ export class PosixKnownPathsLocator extends Locator<BasicEnvInfo> {
}

const iterator = async function* (kind: PythonEnvKind) {
traceVerbose('Searching for interpreters in posix paths locator');
// Filter out pyenv shims. They are not actual python binaries, they are used to launch
// the binaries specified in .python-version file in the cwd. We should not be reporting
// those binaries as environments.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import { traceError, traceVerbose } from '../../../../logging';
* all the environments (global or virtual) in that directory.
*/
async function* getPyenvEnvironments(): AsyncIterableIterator<BasicEnvInfo> {
traceVerbose('Searching for pyenv environments');
const pyenvVersionDir = getPyenvVersionsDir();

const subDirs = getSubDirs(pyenvVersionDir, { resolveSymlinks: true });
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ function getDirFilesLocator(
// rather than in each low-level locator. In the meantime we
// take a naive approach.
async function* iterEnvs(query: PythonLocatorQuery): IPythonEnvsIterator<BasicEnvInfo> {
traceVerbose('Searching for windows path interpreters');
yield* await getEnvs(locator.iterEnvs(query));
traceVerbose('Finished searching for windows path interpreters');
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export class WindowsRegistryLocator extends Locator<BasicEnvInfo> {
// eslint-disable-next-line class-methods-use-this
public iterEnvs(): IPythonEnvsIterator<BasicEnvInfo> {
const iterator = async function* () {
traceVerbose('Searching for windows registry interpreters');
const interpreters = await getRegistryInterpreters();
for (const interpreter of interpreters) {
try {
Expand Down