Read the process list fresh in Kill and TryGetProcessInfo - #777
Closed
SimonCropp wants to merge 1 commit into
Closed
Read the process list fresh in Kill and TryGetProcessInfo#777SimonCropp wants to merge 1 commit into
SimonCropp wants to merge 1 commit into
Conversation
ProcessCleanup filled its list once, in the static constructor, and Refresh had no other caller in the library. So every question about what is running was answered from a snapshot taken the first time anything touched DiffEngine. Inside one process that means a tool launched after first use is invisible for the rest of that process: - Launch then Kill for the same pair logs "No matching commands" and leaves the tool open. - A relaunch never sees the running instance, so a second window opens and another MaxInstance slot goes with it. - An AutoRefresh tool is relaunched instead of being reused and refreshed. - A PID from the startup snapshot may belong to something else entirely by the time Kill terminates it. Refresh at the top of Kill and TryGetProcessInfo. It costs about 14ms on Windows now that CandidateExeNames restricts the command line reads to resolved diff tool images, against a call that is about to start or kill a process. DiffRunnerTests could not have caught this: every test polls through WaitForRunning, which calls Refresh itself, and FakeDiffTool exits on its own after five seconds, so a kill that matched nothing still looked like a kill that worked. The new test refreshes once while the tool is not running, launches, and then asks only the cached query - inside the five seconds, so a pass means it saw the process rather than outlived it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ProcessCleanup filled its list once, in the static constructor, and Refresh had no other caller in the library. So every question about what is running was answered from a snapshot taken the first time anything touched DiffEngine.
Inside one process that means a tool launched after first use is invisible for the rest of that process:
Refresh at the top of Kill and TryGetProcessInfo. It costs about 14ms on Windows now that CandidateExeNames restricts the command line reads to resolved diff tool images, against a call that is about to start or kill a process.
DiffRunnerTests could not have caught this: every test polls through WaitForRunning, which calls Refresh itself, and FakeDiffTool exits on its own after five seconds, so a kill that matched nothing still looked like a kill that worked. The new test refreshes once while the tool is not running, launches, and then asks only the cached query - inside the five seconds, so a pass means it saw the process rather than outlived it.