Bschnurr/fix pylance project env update#8563
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Simplify settings update in PythonLanguageClient by removing the deferred timer and triggering updates immediately. Refactor interpreter change handling in PythonProjectNode for clarity and unsubscribe from Conda discovery events during cleanup. Enhance AutoComplete test in IntellisenseTests to reliably detect completion items and ensure proper event handler cleanup.
| } catch (ObjectDisposedException) { | ||
| } | ||
| TriggerWorkspaceUpdateConfig().DoNotWait(); | ||
| } |
There was a problem hiding this comment.
📍 Python/Product/PythonTools/PythonTools/LanguageServerClient/PythonLanguageClient.cs:492
OnSettingsChanged now runs TriggerWorkspaceUpdateConfig() without the defensive try/catch that the deleted timer callback had. That callback explicitly caught NullReferenceException/ObjectDisposedException with the comment "Field was nulled or service disappeared during teardown" — meaning this failure was observed in practice. While TriggerWorkspaceUpdateConfig self-guards on _disposed || _rpc == null, that guard does not stop GetSettings() from dereferencing a disposed VS option/service mid-teardown, and the exception could now propagate into VS's options-change dispatch. Consider re-adding the defensive try/catch (NullReferenceException)/(ObjectDisposedException) around the call.
There was a problem hiding this comment.
Fixed in 1fa86e1 by restoring the defensive NullReferenceException/ObjectDisposedException handling around the immediate settings update.
| } | ||
| }; | ||
| session.ItemsUpdated += itemsUpdatedHandler; | ||
| completionTask.TrySetResult(session); |
There was a problem hiding this comment.
📍 Python/Tests/Core.UI/IntellisenseTests.cs:49
The handler only subscribes to session.ItemsUpdated and waits for a future event containing executable. If the completion set already contains executable when CompletionTriggered fires (fast/cached path) and no further ItemsUpdated is raised, executableTask never completes and Wait(60000) times out. The old synchronous GetComputedItems(...) could not miss already-present items. Subscribe-then-check: after subscribing, inspect the current computed items once and TrySetResult if executable is already present, closing the race in both directions.
There was a problem hiding this comment.
Fixed in 1fa86e1 by subscribing to ItemsUpdated first and then checking GetComputedItems(...) immediately, so the test handles both already-present and later-updated completion items.
|
Overall the cleanup is sound. Two non-blocking items worth a look: defensive exception handling lost in OnSettingsChanged during teardown, and a potential race in the updated IntellisenseTests if 'executable' is already present when completion triggers. |
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
|
Overall this cleanup looks good. Two robustness follow-ups worth a look: (1) the now-immediate settings update lost the defensive try/catch the old timer callback had for teardown, and (2) the new test only waits on ItemsUpdated and can miss the case where 'executable' is already present when completion triggers. |
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
Restore defensive handling for settings updates during teardown and close the completion item race in the IntelliSense UI test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
rchiodo
left a comment
rchiodo
left a comment
There was a problem hiding this comment.
Approved via Review Center.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
2 participants
PR Classification
Code cleanup and test robustness improvement.
PR Summary
This pull request removes deferred settings update logic in favor of immediate updates and improves test reliability for completion item detection.