Skip to content

Commit 2ed5c03

Browse files
eleanorjboydeleanorjboyd
committed
fix: address PR 1408 fast-path review comments - race condition, failure retry, dedup lambdas, test parameterization
1 parent 41359ea commit 2ed5c03

File tree

8 files changed

+356
-616
lines changed

8 files changed

+356
-616
lines changed

‎src/managers/builtin/sysPythonManager.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import {
2020
import { SysManagerStrings } from '../../common/localize';
2121
import { createDeferred, Deferred } from '../../common/utils/deferred';
2222
import { normalizePath } from '../../common/utils/pathUtils';
23-
import { tryFastPathGet } from '../common/fastPath';
23+
import { getProjectFsPathForScope, tryFastPathGet } from '../common/fastPath';
2424
import { NativePythonFinder } from '../common/nativePythonFinder';
2525
import { getLatest } from '../common/utils';
2626
import {
@@ -148,17 +148,17 @@ export class SysPythonManager implements EnvironmentManager {
148148
async get(scope: GetEnvironmentScope): Promise<PythonEnvironment | undefined> {
149149
const fastResult = await tryFastPathGet({
150150
initialized: this._initialized,
151+
setInitialized: (deferred) => {
152+
this._initialized = deferred;
153+
},
151154
scope,
152155
label: 'system',
153-
getProjectFsPath: (s) => this.api.getPythonProject(s)?.uri.fsPath ?? s.fsPath,
156+
getProjectFsPath: (s) => getProjectFsPathForScope(this.api, s),
154157
getPersistedPath: (fsPath) => getSystemEnvForWorkspace(fsPath),
155158
resolve: (p) => resolveSystemPythonEnvironmentPath(p, this.nativeFinder, this.api, this),
156159
startBackgroundInit: () => this.internalRefresh(false, SysManagerStrings.sysManagerDiscovering),
157160
});
158161
if (fastResult) {
159-
if (fastResult.newDeferred) {
160-
this._initialized = fastResult.newDeferred;
161-
}
162162
return fastResult.env;
163163
}
164164

‎src/managers/builtin/venvManager.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ import { createDeferred, Deferred } from '../../common/utils/deferred';
3535
import { normalizePath } from '../../common/utils/pathUtils';
3636
import { showErrorMessage, showInformationMessage, withProgress } from '../../common/window.apis';
3737
import { findParentIfFile } from '../../features/envCommands';
38-
import { tryFastPathGet } from '../common/fastPath';
38+
import { getProjectFsPathForScope, tryFastPathGet } from '../common/fastPath';
3939
import { NativePythonFinder } from '../common/nativePythonFinder';
4040
import { getLatest, shortVersion, sortEnvironments } from '../common/utils';
4141
import { promptInstallPythonViaUv } from './uvPythonInstaller';
@@ -369,17 +369,17 @@ export class VenvManager implements EnvironmentManager {
369369
async get(scope: GetEnvironmentScope): Promise<PythonEnvironment | undefined> {
370370
const fastResult = await tryFastPathGet({
371371
initialized: this._initialized,
372+
setInitialized: (deferred) => {
373+
this._initialized = deferred;
374+
},
372375
scope,
373376
label: 'venv',
374-
getProjectFsPath: (s) => this.api.getPythonProject(s)?.uri.fsPath ?? s.fsPath,
377+
getProjectFsPath: (s) => getProjectFsPathForScope(this.api, s),
375378
getPersistedPath: (fsPath) => getVenvForWorkspace(fsPath),
376379
resolve: (p) => resolveVenvPythonEnvironmentPath(p, this.nativeFinder, this.api, this, this.baseManager),
377380
startBackgroundInit: () => this.internalRefresh(undefined, false, VenvManagerStrings.venvInitialize),
378381
});
379382
if (fastResult) {
380-
if (fastResult.newDeferred) {
381-
this._initialized = fastResult.newDeferred;
382-
}
383383
return fastResult.env;
384384
}
385385

‎src/managers/common/fastPath.ts‎

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22
// Licensed under the MIT License.
33

44
import { Uri } from 'vscode';
5-
import { GetEnvironmentScope, PythonEnvironment } from '../../api';
5+
import { GetEnvironmentScope, PythonEnvironment, PythonEnvironmentApi } from '../../api';
66
import { traceError, traceWarn } from '../../common/logging';
77
import { createDeferred, Deferred } from '../../common/utils/deferred';
88

@@ -12,6 +12,8 @@ import { createDeferred, Deferred } from '../../common/utils/deferred';
1212
export interface FastPathOptions {
1313
/** The current _initialized deferred (may be undefined if init hasn't started). */
1414
initialized: Deferred<void> | undefined;
15+
/** Updates the manager's _initialized deferred. */
16+
setInitialized: (initialized: Deferred<void> | undefined) => void;
1517
/** The scope passed to get(). */
1618
scope: GetEnvironmentScope;
1719
/** Label for log messages, e.g. 'venv', 'conda'. */
@@ -32,8 +34,16 @@ export interface FastPathOptions {
3234
export interface FastPathResult {
3335
/** The resolved environment. */
3436
env: PythonEnvironment;
35-
/** A new deferred if one was created (caller must assign to _initialized). */
36-
newDeferred?: Deferred<void>;
37+
}
38+
39+
/**
40+
* Gets the fsPath for a scope by preferring the resolved project path when available.
41+
*/
42+
export function getProjectFsPathForScope(
43+
api: Pick<PythonEnvironmentApi, 'getPythonProject'>,
44+
scope: Uri,
45+
): string {
46+
return api.getPythonProject(scope)?.uri.fsPath ?? scope.fsPath;
3747
}
3848

3949
/**
@@ -45,35 +55,44 @@ export interface FastPathResult {
4555
* to fall through to the normal init path.
4656
*/
4757
export async function tryFastPathGet(opts: FastPathOptions): Promise<FastPathResult | undefined> {
48-
if ((!opts.initialized || !opts.initialized.completed) && opts.scope instanceof Uri) {
49-
const fsPath = opts.getProjectFsPath(opts.scope);
50-
const persistedPath = await opts.getPersistedPath(fsPath);
58+
if (!(opts.scope instanceof Uri)) {
59+
return undefined;
60+
}
5161

52-
if (persistedPath) {
53-
try {
54-
const resolved = await opts.resolve(persistedPath);
55-
if (resolved) {
56-
let newDeferred: Deferred<void> | undefined;
57-
// Ensure full init is running in background (may already be in progress)
58-
if (!opts.initialized) {
59-
newDeferred = createDeferred();
60-
const deferred = newDeferred;
61-
Promise.resolve(opts.startBackgroundInit()).then(
62-
() => deferred.resolve(),
63-
(err) => {
64-
traceError(`[${opts.label}] Background initialization failed: ${err}`);
65-
deferred.resolve();
66-
},
67-
);
68-
}
69-
return { env: resolved, newDeferred };
70-
}
71-
} catch (err) {
72-
traceWarn(
73-
`[${opts.label}] Fast path resolve failed for '${persistedPath}', falling back to full init: ${err}`,
74-
);
62+
if (opts.initialized?.completed) {
63+
return undefined;
64+
}
65+
66+
let deferred = opts.initialized;
67+
if (!deferred) {
68+
// Register deferred before any await to avoid concurrent callers starting duplicate inits.
69+
deferred = createDeferred<void>();
70+
opts.setInitialized(deferred);
71+
const deferredRef = deferred;
72+
Promise.resolve(opts.startBackgroundInit()).then(
73+
() => deferredRef.resolve(),
74+
(err) => {
75+
traceError(`[${opts.label}] Background initialization failed: ${err}`);
76+
// Allow subsequent get()/initialize() calls to retry after a background init failure.
77+
opts.setInitialized(undefined);
78+
deferredRef.resolve();
79+
},
80+
);
81+
}
82+
83+
const fsPath = opts.getProjectFsPath(opts.scope);
84+
const persistedPath = await opts.getPersistedPath(fsPath);
85+
86+
if (persistedPath) {
87+
try {
88+
const resolved = await opts.resolve(persistedPath);
89+
if (resolved) {
90+
return { env: resolved };
7591
}
92+
} catch (err) {
93+
traceWarn(`[${opts.label}] Fast path resolve failed for '${persistedPath}', falling back to full init: ${err}`);
7694
}
7795
}
96+
7897
return undefined;
7998
}

‎src/managers/conda/condaEnvManager.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ import { traceError } from '../../common/logging';
2424
import { createDeferred, Deferred } from '../../common/utils/deferred';
2525
import { normalizePath } from '../../common/utils/pathUtils';
2626
import { showErrorMessage, showInformationMessage, withProgress } from '../../common/window.apis';
27-
import { tryFastPathGet } from '../common/fastPath';
27+
import { getProjectFsPathForScope, tryFastPathGet } from '../common/fastPath';
2828
import { NativePythonFinder } from '../common/nativePythonFinder';
2929
import { CondaSourcingStatus } from './condaSourcingUtils';
3030
import {
@@ -263,9 +263,12 @@ export class CondaEnvManager implements EnvironmentManager, Disposable {
263263
async get(scope: GetEnvironmentScope): Promise<PythonEnvironment | undefined> {
264264
const fastResult = await tryFastPathGet({
265265
initialized: this._initialized,
266+
setInitialized: (deferred) => {
267+
this._initialized = deferred;
268+
},
266269
scope,
267270
label: 'conda',
268-
getProjectFsPath: (s) => this.api.getPythonProject(s)?.uri.fsPath ?? s.fsPath,
271+
getProjectFsPath: (s) => getProjectFsPathForScope(this.api, s),
269272
getPersistedPath: (fsPath) => getCondaForWorkspace(fsPath),
270273
resolve: (p) => resolveCondaPath(p, this.nativeFinder, this.api, this.log, this),
271274
startBackgroundInit: () =>
@@ -281,9 +284,6 @@ export class CondaEnvManager implements EnvironmentManager, Disposable {
281284
}),
282285
});
283286
if (fastResult) {
284-
if (fastResult.newDeferred) {
285-
this._initialized = fastResult.newDeferred;
286-
}
287287
return fastResult.env;
288288
}
289289

‎src/managers/pipenv/pipenvManager.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { PipenvStrings } from '../../common/localize';
1818
import { createDeferred, Deferred } from '../../common/utils/deferred';
1919
import { normalizePath } from '../../common/utils/pathUtils';
2020
import { withProgress } from '../../common/window.apis';
21-
import { tryFastPathGet } from '../common/fastPath';
21+
import { getProjectFsPathForScope, tryFastPathGet } from '../common/fastPath';
2222
import { NativePythonFinder } from '../common/nativePythonFinder';
2323
import {
2424
clearPipenvCache,
@@ -258,9 +258,12 @@ export class PipenvManager implements EnvironmentManager {
258258
async get(scope: GetEnvironmentScope): Promise<PythonEnvironment | undefined> {
259259
const fastResult = await tryFastPathGet({
260260
initialized: this._initialized,
261+
setInitialized: (deferred) => {
262+
this._initialized = deferred;
263+
},
261264
scope,
262265
label: 'pipenv',
263-
getProjectFsPath: (s) => this.api.getPythonProject(s)?.uri.fsPath ?? s.fsPath,
266+
getProjectFsPath: (s) => getProjectFsPathForScope(this.api, s),
264267
getPersistedPath: (fsPath) => getPipenvForWorkspace(fsPath),
265268
resolve: (p) => resolvePipenvPath(p, this.nativeFinder, this.api, this),
266269
startBackgroundInit: () =>
@@ -279,9 +282,6 @@ export class PipenvManager implements EnvironmentManager {
279282
),
280283
});
281284
if (fastResult) {
282-
if (fastResult.newDeferred) {
283-
this._initialized = fastResult.newDeferred;
284-
}
285285
return fastResult.env;
286286
}
287287

‎src/managers/pyenv/pyenvManager.ts‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ import { traceError, traceInfo } from '../../common/logging';
2020
import { createDeferred, Deferred } from '../../common/utils/deferred';
2121
import { normalizePath } from '../../common/utils/pathUtils';
2222
import { withProgress } from '../../common/window.apis';
23-
import { tryFastPathGet } from '../common/fastPath';
23+
import { getProjectFsPathForScope, tryFastPathGet } from '../common/fastPath';
2424
import { NativePythonFinder } from '../common/nativePythonFinder';
2525
import { getLatest } from '../common/utils';
2626
import {
@@ -145,9 +145,12 @@ export class PyEnvManager implements EnvironmentManager, Disposable {
145145
async get(scope: GetEnvironmentScope): Promise<PythonEnvironment | undefined> {
146146
const fastResult = await tryFastPathGet({
147147
initialized: this._initialized,
148+
setInitialized: (deferred) => {
149+
this._initialized = deferred;
150+
},
148151
scope,
149152
label: 'pyenv',
150-
getProjectFsPath: (s) => this.api.getPythonProject(s)?.uri.fsPath ?? s.fsPath,
153+
getProjectFsPath: (s) => getProjectFsPathForScope(this.api, s),
151154
getPersistedPath: (fsPath) => getPyenvForWorkspace(fsPath),
152155
resolve: (p) => resolvePyenvPath(p, this.nativeFinder, this.api, this),
153156
startBackgroundInit: () =>
@@ -163,9 +166,6 @@ export class PyEnvManager implements EnvironmentManager, Disposable {
163166
}),
164167
});
165168
if (fastResult) {
166-
if (fastResult.newDeferred) {
167-
this._initialized = fastResult.newDeferred;
168-
}
169169
return fastResult.env;
170170
}
171171

0 commit comments

Comments
 (0)